Skip to content
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

Makes WrappedPromise an ES6 class #113

Merged
merged 1 commit into from
Jun 23, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions es6-wrapped-promise.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
'use strict';

module.exports = (Promise, ensureAslWrapper) => {
// Updates to this class should also be applied to the the ES3 version
// in index.js.
return class WrappedPromise extends Promise {
constructor(executor) {
var context, args;
super(wrappedExecutor);
var promise = this;

try {
executor.apply(context, args);
} catch (err) {
args[1](err);
}

return promise;
function wrappedExecutor(resolve, reject) {
context = this;
args = [wrappedResolve, wrappedReject];

// These wrappers create a function that can be passed a function and an argument to
// call as a continuation from the resolve or reject.
function wrappedResolve(val) {
ensureAslWrapper(promise, false);
return resolve(val);
}

function wrappedReject(val) {
ensureAslWrapper(promise, false);
return reject(val);
}
}
}
}
};
42 changes: 24 additions & 18 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ var shimmer = require('shimmer')
, util = require('util')
;

var v6plus = semver.gte(process.version, '6.0.0');
var v7plus = semver.gte(process.version, '7.0.0');

var net = require('net');
Expand Down Expand Up @@ -391,6 +392,8 @@ if (instrumentPromise) {
function wrapPromise() {
var Promise = global.Promise;

// Updates to this class should also be applied to the the ES6 version
// in es6-wrapped-promise.js.
function wrappedPromise(executor) {
if (!(this instanceof wrappedPromise)) {
return Promise(executor);
Expand All @@ -407,7 +410,7 @@ function wrapPromise() {
try {
executor.apply(context, args);
} catch (err) {
args[1](err)
args[1](err);
}

return promise;
Expand Down Expand Up @@ -438,23 +441,26 @@ function wrapPromise() {
wrap(Promise.prototype, 'chain', wrapThen);
}

var PromiseFunctions = [
'all',
'race',
'reject',
'resolve',
'accept', // Node.js <v7 only
'defer' // Node.js <v7 only
];

PromiseFunctions.forEach(function(key) {
// don't break `in` by creating a key for undefined entries
if (typeof Promise[key] === 'function') {
wrappedPromise[key] = Promise[key];
}
});

global.Promise = wrappedPromise;
if (v6plus) {
global.Promise = require('./es6-wrapped-promise.js')(Promise, ensureAslWrapper);
} else {
var PromiseFunctions = [
'all',
'race',
'reject',
'resolve',
'accept', // Node.js <v7 only
'defer' // Node.js <v7 only
];

PromiseFunctions.forEach(function(key) {
// don't break `in` by creating a key for undefined entries
if (typeof Promise[key] === 'function') {
wrappedPromise[key] = Promise[key];
}
});
global.Promise = wrappedPromise
}

function ensureAslWrapper(promise, overwrite) {
if (!promise.__asl_wrapper || overwrite) {
Expand Down
18 changes: 18 additions & 0 deletions test/native-promises.tap.js
Original file line number Diff line number Diff line change
Expand Up @@ -2198,3 +2198,21 @@ if (typeof Promise.prototype.chain === 'function') {
}
});
}

test('subclasses', function(t) {
if (nodeVersion[0] < 6) {
// class syntax is not supported before node 6.
t.end();
return;
}

// SubclassedPromise does 2 asserts.
t.plan(3);

var SubclassedPromise = require('./promise-subclass.js')(t);

var s = SubclassedPromise.resolve(42).then(function(val) {
t.strictEqual(val, 42);
t.end();
});
});
11 changes: 11 additions & 0 deletions test/promise-subclass.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
'use strict';

module.exports = (tap) => {
return class SubclassedPromise extends Promise {
then(onSuccess, onReject) {
tap.type(onSuccess, 'function');
tap.type(onReject, 'undefined');
return Promise.prototype.then.call(this, onSuccess, onReject);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation 👍 I'm fine with just merging this then. We can always modify the style in master if we want

}
};
};