From 50357b5f0a1b0ba91100e60d724aead83aa8fb11 Mon Sep 17 00:00:00 2001 From: Juan Carlos Medina Date: Mon, 6 Aug 2018 12:13:25 -0700 Subject: [PATCH 01/14] Backfill environment tests for Promise It has a minimal Promise interface. --- test/environment-test.js | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 test/environment-test.js diff --git a/test/environment-test.js b/test/environment-test.js new file mode 100644 index 0000000..98fa527 --- /dev/null +++ b/test/environment-test.js @@ -0,0 +1,24 @@ +import { assert } from 'chai'; +import '../src/environment'; + +describe('environment', () => { + describe('global promise', () => { + it('has ES6 methods', () => { + assert.isFunction(global.Promise.prototype.then); + assert.isFunction(global.Promise.prototype.catch); + assert.isFunction(global.Promise.prototype.constructor); + }); + + it('has ES6 static methods', () => { + assert.isFunction(global.Promise.all); + assert.isFunction(global.Promise.race); + assert.isFunction(global.Promise.resolve); + assert.isFunction(global.Promise.reject); + assert.isFunction(global.Promise.cast); + }); + + it('removes extraneous methods', () => { + assert.isUndefined(global.Promise.promisifyAll); + }); + }); +}); From 0c9df7956afaac0243337080b83a8b7e63528a3d Mon Sep 17 00:00:00 2001 From: Juan Carlos Medina Date: Mon, 6 Aug 2018 12:22:35 -0700 Subject: [PATCH 02/14] Ensure environment setup does not pollute bluebird module --- src/environment.js | 13 ++++++++----- test/environment-test.js | 6 ++++++ 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/environment.js b/src/environment.js index 80ad23d..56c61cf 100644 --- a/src/environment.js +++ b/src/environment.js @@ -18,9 +18,12 @@ function toFastProperties(obj) { (function () {}).prototype = obj; } -Object.keys(Promise.prototype).filter(isNotMethod).forEach(del(Promise.prototype)); -Object.keys(Promise).filter(isNotMethod).forEach(del(Promise)); -toFastProperties(Promise); -toFastProperties(Promise.prototype); +const PromiseCopy = Object.assign({}, Promise); +PromiseCopy.prototype = Object.assign({}, Promise.prototype); -global.Promise = Promise; +Object.keys(Promise.prototype).filter(isNotMethod).forEach(del(PromiseCopy.prototype)); +Object.keys(Promise).filter(isNotMethod).forEach(del(PromiseCopy)); +toFastProperties(PromiseCopy); +toFastProperties(PromiseCopy.prototype); + +global.Promise = PromiseCopy; diff --git a/test/environment-test.js b/test/environment-test.js index 98fa527..62b3438 100644 --- a/test/environment-test.js +++ b/test/environment-test.js @@ -1,7 +1,13 @@ import { assert } from 'chai'; +import Promise from 'bluebird'; import '../src/environment'; + describe('environment', () => { + it('does not modify the Promise package', () => { + assert.notStrictEqual(Promise, global.Promise); + }); + describe('global promise', () => { it('has ES6 methods', () => { assert.isFunction(global.Promise.prototype.then); From b10b37d0831978ce0a25394c1dc382c1211a18fc Mon Sep 17 00:00:00 2001 From: Juan Carlos Medina Date: Mon, 6 Aug 2018 12:27:29 -0700 Subject: [PATCH 03/14] Tidy up whitespace --- test/environment-test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/environment-test.js b/test/environment-test.js index 62b3438..41789e6 100644 --- a/test/environment-test.js +++ b/test/environment-test.js @@ -2,7 +2,6 @@ import { assert } from 'chai'; import Promise from 'bluebird'; import '../src/environment'; - describe('environment', () => { it('does not modify the Promise package', () => { assert.notStrictEqual(Promise, global.Promise); From 63f6a01cbcbb047f335ff43fd00268ea0bab9771 Mon Sep 17 00:00:00 2001 From: Juan Carlos Medina Date: Mon, 6 Aug 2018 13:27:31 -0700 Subject: [PATCH 04/14] Do not modify bluebird Promises These are fast. Do we need them to be faster? --- src/environment.js | 26 +------------------------- test/environment-test.js | 9 --------- 2 files changed, 1 insertion(+), 34 deletions(-) diff --git a/src/environment.js b/src/environment.js index 56c61cf..dfc8a83 100644 --- a/src/environment.js +++ b/src/environment.js @@ -2,28 +2,4 @@ import 'airbnb-js-shims'; import Promise from 'bluebird'; -const es6methods = ['then', 'catch', 'constructor']; -const es6StaticMethods = ['all', 'race', 'resolve', 'reject', 'cast']; - -function isNotMethod(name) { - return !(es6methods.includes(name) || es6StaticMethods.includes(name) || name.charAt(0) === '_'); -} - -function del(obj) { - /* eslint no-param-reassign: 0 */ - return (key) => { delete obj[key]; }; -} - -function toFastProperties(obj) { - (function () {}).prototype = obj; -} - -const PromiseCopy = Object.assign({}, Promise); -PromiseCopy.prototype = Object.assign({}, Promise.prototype); - -Object.keys(Promise.prototype).filter(isNotMethod).forEach(del(PromiseCopy.prototype)); -Object.keys(Promise).filter(isNotMethod).forEach(del(PromiseCopy)); -toFastProperties(PromiseCopy); -toFastProperties(PromiseCopy.prototype); - -global.Promise = PromiseCopy; +global.Promise = Promise; diff --git a/test/environment-test.js b/test/environment-test.js index 41789e6..f9bfd2d 100644 --- a/test/environment-test.js +++ b/test/environment-test.js @@ -1,12 +1,7 @@ import { assert } from 'chai'; -import Promise from 'bluebird'; import '../src/environment'; describe('environment', () => { - it('does not modify the Promise package', () => { - assert.notStrictEqual(Promise, global.Promise); - }); - describe('global promise', () => { it('has ES6 methods', () => { assert.isFunction(global.Promise.prototype.then); @@ -21,9 +16,5 @@ describe('environment', () => { assert.isFunction(global.Promise.reject); assert.isFunction(global.Promise.cast); }); - - it('removes extraneous methods', () => { - assert.isUndefined(global.Promise.promisifyAll); - }); }); }); From d357426a7b38513954d5be666cfdce9d0845cf49 Mon Sep 17 00:00:00 2001 From: Juan Carlos Medina Date: Tue, 7 Aug 2018 13:37:41 -0700 Subject: [PATCH 05/14] Implement StrictPromise to restrict Bluebird Promise interface Related issue https://github.com/airbnb/hypernova/issues/134 --- src/environment.js | 2 +- src/utils/strict-promise.js | 69 +++++++++++++++++++++++++++++++ test/environment-test.js | 1 - test/utils/strict-promise-test.js | 54 ++++++++++++++++++++++++ 4 files changed, 124 insertions(+), 2 deletions(-) create mode 100644 src/utils/strict-promise.js create mode 100644 test/utils/strict-promise-test.js diff --git a/src/environment.js b/src/environment.js index dfc8a83..1b4e6c1 100644 --- a/src/environment.js +++ b/src/environment.js @@ -1,5 +1,5 @@ /* eslint func-names:0 no-extra-parens:0 */ import 'airbnb-js-shims'; -import Promise from 'bluebird'; +import Promise from './utils/strict-promise'; global.Promise = Promise; diff --git a/src/utils/strict-promise.js b/src/utils/strict-promise.js new file mode 100644 index 0000000..d0f567c --- /dev/null +++ b/src/utils/strict-promise.js @@ -0,0 +1,69 @@ +/* eslint-disable no-underscore-dangle */ + +import Promise from 'bluebird'; + +export default class StrictPromise { + constructor(executor) { + this._promise = new Promise(executor); + } + + static all(iterable) { + const newPromise = Promise.all(iterable); + + const strictPromise = new StrictPromise((resolve, reject) => { + newPromise.then(resolve, reject); + }); + + return strictPromise; + } + + static race(iterable) { + const newPromise = Promise.race(iterable); + + const strictPromise = new StrictPromise((resolve, reject) => { + newPromise.then(resolve, reject); + }); + + return strictPromise; + } + + static reject(value) { + const newPromise = Promise.reject(value); + + const strictPromise = new StrictPromise((resolve, reject) => { + newPromise.then(resolve, reject); + }); + + return strictPromise; + } + + static resolve(value) { + const newPromise = Promise.resolve(value); + + const strictPromise = new StrictPromise((resolve, reject) => { + newPromise.then(resolve, reject); + }); + + return strictPromise; + } + + then(onFulfilled, onRejected) { + const newPromise = this._promise.then(onFulfilled, onRejected); + + const strictPromise = new StrictPromise((resolve, reject) => { + newPromise.then(resolve, reject); + }); + + return strictPromise; + } + + catch(onRejected) { + const newPromise = this._promise.catch(onRejected); + + const strictPromise = new StrictPromise((resolve, reject) => { + newPromise.then(resolve, reject); + }); + + return strictPromise; + } +} diff --git a/test/environment-test.js b/test/environment-test.js index f9bfd2d..21f1a9e 100644 --- a/test/environment-test.js +++ b/test/environment-test.js @@ -14,7 +14,6 @@ describe('environment', () => { assert.isFunction(global.Promise.race); assert.isFunction(global.Promise.resolve); assert.isFunction(global.Promise.reject); - assert.isFunction(global.Promise.cast); }); }); }); diff --git a/test/utils/strict-promise-test.js b/test/utils/strict-promise-test.js new file mode 100644 index 0000000..61438b9 --- /dev/null +++ b/test/utils/strict-promise-test.js @@ -0,0 +1,54 @@ +import { assert } from 'chai'; +import StrictPromise from '../../src/utils/strict-promise'; + +describe('StrictPromise', () => { + describe('thennable', () => { + it('has a fulfillment callback', (done) => { + let resolveWith; + const message = 'rejected'; + + const promise = new StrictPromise((resolve, reject) => { + resolveWith = reject; + }); + + promise.catch((resolvedWith) => { + assert.isStrictEqual(resolvedWith, message); + done(); + }); + + resolveWith(); + }); + + it('has a rejection callback', (done) => { + let rejectWith; + const message = 'rejected'; + + const promise = new StrictPromise((resolve, reject) => { + rejectWith = reject; + }); + + promise.catch((rejectedWith) => { + assert.isStrictEqual(rejectedWith, message); + done(); + }); + + rejectWith(); + }); + }); + + it('is catchable', (done) => { + let rejectWith; + const message = 'rejected'; + + const promise = new StrictPromise((resolve, reject) => { + rejectWith = reject; + }); + + promise.catch((rejectedWith) => { + assert.isStrictEqual(rejectedWith, message); + done(); + }); + + rejectWith(); + }); +}); From 62e12345e0f7469989762d9c8ed9ac9318928e80 Mon Sep 17 00:00:00 2001 From: Juan Carlos Medina Date: Tue, 7 Aug 2018 13:51:19 -0700 Subject: [PATCH 06/14] Use appropriate import variable name for StrictPromise --- src/environment.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/environment.js b/src/environment.js index 1b4e6c1..59e9936 100644 --- a/src/environment.js +++ b/src/environment.js @@ -1,5 +1,5 @@ /* eslint func-names:0 no-extra-parens:0 */ import 'airbnb-js-shims'; -import Promise from './utils/strict-promise'; +import StrictPromise from './utils/strict-promise'; -global.Promise = Promise; +global.Promise = StrictPromise; From 6ec54da8dae56dedc3cb3e074dd4617f1e317e7d Mon Sep 17 00:00:00 2001 From: Juan Carlos Medina Date: Tue, 7 Aug 2018 14:35:02 -0700 Subject: [PATCH 07/14] Fix specs: fulfillment callback uses resolve. rejected callback is second parameter to `then` --- test/utils/strict-promise-test.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/utils/strict-promise-test.js b/test/utils/strict-promise-test.js index 61438b9..78b5d74 100644 --- a/test/utils/strict-promise-test.js +++ b/test/utils/strict-promise-test.js @@ -5,13 +5,13 @@ describe('StrictPromise', () => { describe('thennable', () => { it('has a fulfillment callback', (done) => { let resolveWith; - const message = 'rejected'; + const message = 'fulfilled'; - const promise = new StrictPromise((resolve, reject) => { - resolveWith = reject; + const promise = new StrictPromise((resolve) => { + resolveWith = resolve; }); - promise.catch((resolvedWith) => { + promise.then((resolvedWith) => { assert.isStrictEqual(resolvedWith, message); done(); }); @@ -27,7 +27,7 @@ describe('StrictPromise', () => { rejectWith = reject; }); - promise.catch((rejectedWith) => { + promise.then(() => {}, (rejectedWith) => { assert.isStrictEqual(rejectedWith, message); done(); }); From 3081a25d771af6f01647ad6692c381e3b056273e Mon Sep 17 00:00:00 2001 From: Juan Carlos Medina Date: Tue, 7 Aug 2018 14:39:29 -0700 Subject: [PATCH 08/14] Backfill specs: StrictPromise.resolve --- test/utils/strict-promise-test.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/utils/strict-promise-test.js b/test/utils/strict-promise-test.js index 78b5d74..ca89e0f 100644 --- a/test/utils/strict-promise-test.js +++ b/test/utils/strict-promise-test.js @@ -2,6 +2,17 @@ import { assert } from 'chai'; import StrictPromise from '../../src/utils/strict-promise'; describe('StrictPromise', () => { + describe('static resolve', () => { + it('resolves with a given reason', (done) => { + const message = 'fulfilled'; + + StrictPromise.reject(message).catch((resolvedWith) => { + assert.isStrictEqual(resolvedWith, message); + done(); + }); + }); + }); + describe('thennable', () => { it('has a fulfillment callback', (done) => { let resolveWith; From 8c3f894ae0f9a1b4ef5004342c4d586270e24dac Mon Sep 17 00:00:00 2001 From: Juan Carlos Medina Date: Tue, 7 Aug 2018 14:39:51 -0700 Subject: [PATCH 09/14] Backfill specs: StrictPromise.reject --- test/utils/strict-promise-test.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/utils/strict-promise-test.js b/test/utils/strict-promise-test.js index ca89e0f..66a74c8 100644 --- a/test/utils/strict-promise-test.js +++ b/test/utils/strict-promise-test.js @@ -2,6 +2,17 @@ import { assert } from 'chai'; import StrictPromise from '../../src/utils/strict-promise'; describe('StrictPromise', () => { + describe('static reject', () => { + it('rejects with a given reason', (done) => { + const message = 'rejected'; + + StrictPromise.reject(message).catch((rejectedWith) => { + assert.isStrictEqual(rejectedWith, message); + done(); + }); + }); + }); + describe('static resolve', () => { it('resolves with a given reason', (done) => { const message = 'fulfilled'; From 6512c808356d0df7055a5987664ed1a840e7be1c Mon Sep 17 00:00:00 2001 From: Juan Carlos Medina Date: Tue, 7 Aug 2018 14:50:14 -0700 Subject: [PATCH 10/14] `npm test` also runs test in nested folders e.g. utils/strict-promise-test.js --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 9cfdb7c..967b78e 100644 --- a/package.json +++ b/package.json @@ -14,7 +14,7 @@ "pretests-only": "npm run build", "tests-only": "npm run test:quick", "precoverage": "npm run build", - "coverage": "babel-node node_modules/.bin/istanbul cover --report html node_modules/.bin/_mocha -- -R tap test/init.js test/*-test.js", + "coverage": "babel-node node_modules/.bin/istanbul cover --report html node_modules/.bin/_mocha -- -R tap test/init.js test/*-test.js test/**/*-test.js", "postcoverage": "npm run cover:check", "cover:check": "istanbul check-coverage && echo code coverage thresholds met, achievement unlocked!", "test:quick": "babel-node node_modules/.bin/_mocha -R tap test/init.js test/*-test.js" From be5309f95d1dd1acb9135d9ae7768960e18d6362 Mon Sep 17 00:00:00 2001 From: Juan Carlos Medina Date: Tue, 7 Aug 2018 14:53:34 -0700 Subject: [PATCH 11/14] Fix specs: chai API issues => async test issues => ensure tests are running `isStrictEqual` is not part of the API: replaced with `strictEqual` Ensure promise is either rejected or resolved correctly. This was entirely missed because I forgot to ensure the tests were running. I was breaking the test suite by removing code from StrictPromise -- but not ensuring the strict-promise-test was failing appropriately. --- test/utils/strict-promise-test.js | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/test/utils/strict-promise-test.js b/test/utils/strict-promise-test.js index 66a74c8..04b72c2 100644 --- a/test/utils/strict-promise-test.js +++ b/test/utils/strict-promise-test.js @@ -7,7 +7,7 @@ describe('StrictPromise', () => { const message = 'rejected'; StrictPromise.reject(message).catch((rejectedWith) => { - assert.isStrictEqual(rejectedWith, message); + assert.strictEqual(rejectedWith, message); done(); }); }); @@ -17,8 +17,8 @@ describe('StrictPromise', () => { it('resolves with a given reason', (done) => { const message = 'fulfilled'; - StrictPromise.reject(message).catch((resolvedWith) => { - assert.isStrictEqual(resolvedWith, message); + StrictPromise.resolve(message).then((resolvedWith) => { + assert.strictEqual(resolvedWith, message); done(); }); }); @@ -34,11 +34,11 @@ describe('StrictPromise', () => { }); promise.then((resolvedWith) => { - assert.isStrictEqual(resolvedWith, message); + assert.strictEqual(resolvedWith, message); done(); }); - resolveWith(); + resolveWith(message); }); it('has a rejection callback', (done) => { @@ -50,11 +50,11 @@ describe('StrictPromise', () => { }); promise.then(() => {}, (rejectedWith) => { - assert.isStrictEqual(rejectedWith, message); + assert.strictEqual(rejectedWith, message); done(); }); - rejectWith(); + rejectWith(message); }); }); @@ -65,12 +65,11 @@ describe('StrictPromise', () => { const promise = new StrictPromise((resolve, reject) => { rejectWith = reject; }); - promise.catch((rejectedWith) => { - assert.isStrictEqual(rejectedWith, message); + assert.strictEqual(rejectedWith, message); done(); }); - rejectWith(); + rejectWith(message); }); }); From 2590dc5211a7284013a05ea3d1220e541a0fddc7 Mon Sep 17 00:00:00 2001 From: Juan Carlos Medina Date: Tue, 7 Aug 2018 16:12:33 -0700 Subject: [PATCH 12/14] Backfill specs: StrictPromise.race --- test/utils/strict-promise-test.js | 38 +++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/test/utils/strict-promise-test.js b/test/utils/strict-promise-test.js index 04b72c2..c9d6915 100644 --- a/test/utils/strict-promise-test.js +++ b/test/utils/strict-promise-test.js @@ -2,6 +2,44 @@ import { assert } from 'chai'; import StrictPromise from '../../src/utils/strict-promise'; describe('StrictPromise', () => { + describe('static race', () => { + it('resolves with the first resolved iterable', (done) => { + let resolveWith; + const message = 'It is a resolved race!'; + + StrictPromise.race([ + new StrictPromise(() => {}), + new StrictPromise((resolve) => { + resolveWith = resolve; + }), + new StrictPromise(() => {}), + ]).then((resolvedWith) => { + assert.strictEqual(resolvedWith, message); + done(); + }); + + resolveWith(message); + }); + + it('rejects with the first rejected iterable', (done) => { + let rejectWith; + const message = 'It is a rejected race!'; + + StrictPromise.race([ + new StrictPromise(() => {}), + new StrictPromise((resolve, reject) => { + rejectWith = reject; + }), + new StrictPromise(() => {}), + ]).then(() => {}, (rejectedWith) => { + assert.strictEqual(rejectedWith, message); + done(); + }); + + rejectWith(message); + }); + }); + describe('static reject', () => { it('rejects with a given reason', (done) => { const message = 'rejected'; From 9baf25df4fcae061fdf58631a785a93258e59910 Mon Sep 17 00:00:00 2001 From: Juan Carlos Medina Date: Tue, 7 Aug 2018 16:25:18 -0700 Subject: [PATCH 13/14] Backfill specs: StrictPromise.all --- test/utils/strict-promise-test.js | 56 +++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/test/utils/strict-promise-test.js b/test/utils/strict-promise-test.js index c9d6915..d407c19 100644 --- a/test/utils/strict-promise-test.js +++ b/test/utils/strict-promise-test.js @@ -2,6 +2,62 @@ import { assert } from 'chai'; import StrictPromise from '../../src/utils/strict-promise'; describe('StrictPromise', () => { + describe('static all', () => { + it('resolves when the iterable is empty', (done) => { + StrictPromise.all([]).then(() => { + done(); + }); + }); + + it('resolves when the whole iterable is resolved', (done) => { + const resolveWiths = []; + const messages = [ + 'First resolved.', + 'Second resolved.', + 'Third resolved', + ]; + + StrictPromise.all([ + new StrictPromise((resolve) => { + resolveWiths.push(resolve); + }), + new StrictPromise((resolve) => { + resolveWiths.push(resolve); + }), + new StrictPromise((resolve) => { + resolveWiths.push(resolve); + }), + ]).then((resolvedWiths) => { + assert.strictEqual(resolvedWiths[0], messages[0]); + assert.strictEqual(resolvedWiths[1], messages[1]); + assert.strictEqual(resolvedWiths[2], messages[2]); + done(); + }); + + resolveWiths[0](messages[0]); + resolveWiths[1](messages[1]); + resolveWiths[2](messages[2]); + }); + + it('rejects with the first rejected iterable', (done) => { + let rejectWith; + const message = 'Only one rejected.'; + + StrictPromise.all([ + new StrictPromise(() => {}), + new StrictPromise((resolve, reject) => { + rejectWith = reject; + }), + new StrictPromise(() => {}), + ]).then(() => {}, (rejectedWith) => { + assert.strictEqual(rejectedWith, message); + done(); + }); + + rejectWith(message); + }); + }); + describe('static race', () => { it('resolves with the first resolved iterable', (done) => { let resolveWith; From af9a5ed6a7235aa3601852bb3758025aa9971eec Mon Sep 17 00:00:00 2001 From: Juan Carlos Medina Date: Tue, 7 Aug 2018 16:25:27 -0700 Subject: [PATCH 14/14] Tidy up --- test/utils/strict-promise-test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/utils/strict-promise-test.js b/test/utils/strict-promise-test.js index d407c19..41ebae7 100644 --- a/test/utils/strict-promise-test.js +++ b/test/utils/strict-promise-test.js @@ -159,6 +159,7 @@ describe('StrictPromise', () => { const promise = new StrictPromise((resolve, reject) => { rejectWith = reject; }); + promise.catch((rejectedWith) => { assert.strictEqual(rejectedWith, message); done();