Upload endpoints #16
Upload endpoints #16
Conversation
src/data/data.js
Outdated
| @@ -32,19 +32,24 @@ class Data extends Base { | |||
| super(...params); | |||
|
|
|||
| /** @type {Metrics} */ | |||
| this.metrics = new Metrics(this); | |||
| this.Metrics = new Metrics(this); | |||
| this.metrics = this.Metrics; | |||
There was a problem hiding this comment.
Why do you still need these? Just so this doesn't break backwards compatibility?
There was a problem hiding this comment.
Yeah, just so there'd be one less big change to make if you were updating from 1.x (you already have to remove all the data.data accessors)
| * @example | ||
| * const { Video } = new Mux(accessToken, secret); | ||
| * | ||
| * // Create an upload |
There was a problem hiding this comment.
Shouldn't this be around line 38... or just removed?
There was a problem hiding this comment.
It's just an example for using the class module, but I was just being consistent with the other modules and don't feel too strongly about it.
src/video/resources/uploads.js
Outdated
| * const { Video } = new Mux(accessToken, secret); | ||
| * | ||
| * // Delete an upload | ||
| * Video.Uploads.remove(uploadId); |
There was a problem hiding this comment.
Do we use remove elsewhere for delete things? Like... why not `delete?
Cool if we already use remove elsewhere, just asking
There was a problem hiding this comment.
Yeah, all the other modules used remove.
There was a problem hiding this comment.
delete is a reserved word in Javascript and i believe we were using deleteAsset or removeAsset before, but there was a request to keep it to one word for consistency.
| const createdUploads = []; // These are uploads we'll clean up when it's all done. | ||
|
|
||
|
|
||
| before(async () => { |
There was a problem hiding this comment.
Why do you do this? You have a test specifically for this, and then create one directly in each test where you need to do things...
There was a problem hiding this comment.
Except for the get call... why not just create it in the get test?
| expect(TestData.Errors).to.be.an.instanceof(Errors); | ||
| expect(TestData.Exports).to.be.an.instanceof(Exports); | ||
| expect(TestData.VideoViews).to.be.an.instanceof(VideoViews); | ||
| expect(TestData.Filters).to.be.an.instanceof(Filters); |
There was a problem hiding this comment.
If we're keeping the lower-cased versions around for backwards compatibility, should we add tests to make sure those exist so we don't accidentally remove them for now?
| expect(TestVideo.Assets).to.be.an.instanceof(Assets); | ||
| expect(TestVideo.LiveStreams).to.be.an.instanceof(LiveStreams); | ||
| expect(TestVideo.LiveStreams).to.be.an.instanceof(LiveStreams); | ||
| expect(TestVideo.Uploads).to.be.an.instanceof(Uploads); |
There was a problem hiding this comment.
Same here with the lower-cased ones...
| @@ -22,7 +22,7 @@ yarn add @mux/mux-node | |||
| ``` | |||
|
|
|||
| ## Releases | |||
| The latest **stable** release is `1.1.1` | |||
| The latest **stable** release is `2.0.0` | |||
There was a problem hiding this comment.
Ooh if this is a major version bump... let's just kill the lower-cased versions of things?
There was a problem hiding this comment.
Yeah, that's fair, I was trying to make it one less messy thing, but this is already an annoying upgrade. 😢
src/video/resources/assets.js
Outdated
| @@ -136,8 +132,7 @@ class Assets extends Base { | |||
| * @returns {Promise} - Returns a resolved Promise with a response from the Mux API | |||
| * | |||
| * @example | |||
| * const muxClient = new Mux(accessToken, secret); | |||
| * const { Video } = muxClient; | |||
| * const { Video }= new Mux(accessToken, secret); | |||
There was a problem hiding this comment.
you're missing a space after { Video } and before the =
No description provided.