-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[API-18] New file service endpoints and workflow support #55
Conversation
case bodyTypes.BUFFER: | ||
await this._uploadBufferFile(body, fileId, size); | ||
break; | ||
|
||
case bodyTypes.STREAM: | ||
this._chunks = await this._uploadStreamFile(body, fileId); | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to support both options? And wouldn't it be easier to just pass the file path, instead of a stream / buffer? This is how we do it in the other SDKs but I'm not sure about the history of this SDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Node, Buffers are usually used for small size files. Streams are commonly used for larger ones.
Although we could ask for the path, I would prefer to leave this decision on the developer hands. That way we could avoid having the calculate file size each time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On this SDK in particular, we have always expected a Buffer or Stream file body. Not a path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FS stuff looks good to me. The only changes I would say are not merging the node-version upgrade as part of this and have a separate ticket so it can be tested for publishing and any side-effects 😄
Will not need this anymore as this PR depends on the new node-versions 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allright looking good! Just some minor concerns and I would leave the cli sample out of this PR.
body = queryString.stringify(params); | ||
// Some older endpoints require this | ||
options.additionalHeaders = { | ||
...options.additionalHeaders, | ||
'Content-Type': 'application/x-www-form-urlencoded' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this work for requests that don't have a body (such as /api/v4/media/{id}/save/{fileId}
)?
Wondering since in the java sdk this would result in an error in the framework used there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. As a matter of fact, axios always sends an empty body by default.
const bodyType = bodyTypes.get(body); | ||
|
||
if (bodyType === bodyTypes.BUFFER) { | ||
return body.length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't we want to (re)throw that exception instead of returning length
(which would be 0) in that case?
2ca136d
to
0147c07
Compare
* ESLint config clean up * Gulp replaced with Rollup * Beblified modules * Discard browser builds for now * Identation fix * UUID package added * Do not ignore package-lock files * For no-console and single-quote * Generate a package-lock file and track it * Generate docs through NPM scripts * Old dependencies removed * Don't let rollup include external dependencies * Ignore ENV file * Unnecessary Jasmine files removed * Don't include the docs nor the coverage on the NPM bundle * Babel config extracted from package file * Main API file extracted from SDK file * Constants and utils file created * New bynder module * Use optional chaining to check if file body exists * WIP * Docs command fix * ESlint config fixed * Unrequired proper-url-join package removed * Use an axios instance instead of creating a new one everytime * New mocked functions helpers * WIP * Build support for Node 12, 13, and 14 added * Test fixed * API class tests * Package files added * Use the standarized UA name * Tests fixed * Catch request errors correctly + API class documentation improvements * New FS workflow w/ prepare endpoint support * Version bump * New metapropertyOptions endpoint support Closes #51 * Media download URL endpoint support added Closes #49 * Save asset method fixed * README updated with new methods * File chunks method added to README file * Ignore lines for CC * USe Jest's spy objects for a cleaner implementation * Chunk uploader bug fixed * Sample script replaced * Front-end related code removed * Pretest script fixed * Use existing scripts for consistency * Typo fix * Endpoint specific headers can sent through the API client * Utils tests cases * Include the content sha and media ID * More tests cases * SHA256 Hex generator moved to the utils file * Send intent and SHA256 on finalise upload endpoint request * Moar tests added * Docs updated * Return arrays as data attr * URL namespaces fixed * Only encode files when hitting a non V6 and V7 endpoint * Moar tests * Documentation improvements * Send headers separately from body payload * Correct finalise endpoint + header improvements * Last set of tests added * Review changes * Unused deps removed * Run GitHub actions within supported NodeJS versions * Readable stream file body are supported * isomorphic-form-data removed * Support for stream uploads added * Debug lines removed * Fiel intent removed * Form data moved * Don't use Yarn * Smol documentation fixes * Return an array, and no headers, when an array is responded * Detect V4 and V5 only routes
Closes #54, #53, #52, #50, #47, #46, #45, and #59.