Makes WrappedPromise an ES6 class #113
Conversation
|
I don't think this will work as a valid solution because the file still needs to parse in old versions of node that do not support the class syntax. It should be possible to make subclassing work without using the class syntax |
|
It looks like subclassing promises without the class syntax may require assigning to |
|
@hayes If necessary this could be pulled to a different file that is conditionally required if running a version of node that supports classes. I think that would be an ugly approach and am definitely open to other options but am not aware of any other solutions. |
|
Oops, looks like there was a race in the last two comments. I can pull this into a separate file if that is the best way forward. |
|
I think that would be a good next step. I'll not sure what versions of node support promises but not class syntax, so that would be something worth checking out as well. I wont have time until later this week, but I could look into that if you want |
|
@hayes I've got things passing on everything but v7 and the failures there seem unrelated. |
|
Though I haven't looked at the CI error in detail, I wouldn't be surprised if the reason why this fails in 7 is because of this: nodejs/node#12861 |
|
If everyone is ok with merging #117, then we can rebase this PR to check that the tests pass correctly. |
es6-wrapped-promise.js
Outdated
| } | ||
|
|
||
| return promise; | ||
| function wrappedExecutor(resolve, reject) { |
There was a problem hiding this comment.
In case we ever need to make a change to this function, I'd probably add a comment noting that this is a duplicate of the function in index.js. And I'd also make a similar comment by the function in index.js. Else they might deviate by a mistake at some point
There was a problem hiding this comment.
Good point! I've added the comments.
|
This looks great, I think it should pass CI once this is rebased on the changes @watson made to travis.yml I am going to leave this here for @othiym23 to merge since it is a more significant change than just testing/ci, and I don't have access to cut a new release anyways. Thanks for working on this! |
|
I was mistaken, ci still won't pass due to an unrelated issue, but I think this change still looks good to me |
es6-wrapped-promise.js
Outdated
| try { | ||
| executor.apply(context, args); | ||
| } catch (err) { | ||
| args[1](err) |
There was a problem hiding this comment.
I tried to keep naming and style the same as the ES3 counterpart. If we want to address general style issues there are a few other things I would want clean up too.
There was a problem hiding this comment.
I'd say it's a mistake that there were no semicolon in the ES3 counterpart. We should probably fix that as well 😃
There was a problem hiding this comment.
Fixed in both places.
es6-wrapped-promise.js
Outdated
| return promise; | ||
| function wrappedExecutor(resolve, reject) { | ||
| context = this; | ||
| args = [wrappedAccept, wrappedReject]; |
There was a problem hiding this comment.
nit: either rename resolve to accept or rename wrappedAccept to wrappedResolve.
There was a problem hiding this comment.
An effort was made recently to rename all accept to resolve. A few places this was missed, but I made a PR to address this: #108
I didn't have commit access at the time, otherwise I could probably just have merged it my self as it's just naming. It have been sitting there for over a month now with no comments so I took the liberty to just merge it now. I hope that's ok 😅
There was a problem hiding this comment.
I've renamed this to wrappedResolve.
|
@watson not my PR (and the branch is not on this repo) so I can't rebase it myself, but just merged the fix for tests on node 8, so after a rebase we should have a green build. |
|
@hayes ah my bad 😄 But nice to know that we now have a stable CI build 👍 |
This allow for correct dispatch of method calls made on ES6 classes that subclass promise. Fixes: #112
|
All rebased, looks like the CI is green (with the new allowed failures). |
| then(onSuccess, onReject) { | ||
| tap.type(onSuccess, 'function'); | ||
| tap.type(onReject, 'undefined'); | ||
| return Promise.prototype.then.call(this, onSuccess, onReject); |
There was a problem hiding this comment.
I'm not sure if it makes a difference in this case as I'm not that familiar with the new class syntax, but is there a difference between Promise.prototype.then.call() and super.then()? I haven't seen the two styles mixed like this before
There was a problem hiding this comment.
I just checked out the branch to test it and the test passed using super as well 😃
I'm not sure if it's a big deal - if not, just leave it as is and feel free to merge on my account 👍
There was a problem hiding this comment.
Here they should be effectively the same. Using super you are guaranteed that your getting tge then you want, using this approach Promise, or Promise.prototype could be changed out from underneath you. In a test this shouldnt be an issue, but would be a bad idea if we were using this in our wrapping code
There was a problem hiding this comment.
Thanks for the explanation 👍 I'm fine with just merging this then. We can always modify the style in master if we want
|
What are the next steps to getting this landed/released? |
|
I just took the liberty to merge this as there didn't seem to be any objections (I hope that's ok with everybody 😅). Next step is to cut a new version and publish. I do have the publish bit, so I can do it if nobody mind? |
|
After talking to @othiym23 I have now released this as v0.6.6 on npm 😅🎉 |
|
Thanks! 😄 |
This allow for correct dispatch of method calls made on ES6 classes that
subclass promise.
Fixes: #112