Issue 29333514: Issue 3515 - Added polyfill for ES6 promises (Closed)
|
Created:
Jan. 14, 2016, 5:19 p.m. by Sebastian Noack Modified:
Jan. 19, 2016, 4:41 p.m. Reviewers:
kzar Visibility:
Public. |
DescriptionIssue 3515 - Added polyfill for ES6 promises
Patch Set 1 #
Total comments: 22
Patch Set 2 : Fixed corner cases with Promise.all() #Patch Set 3 : Fixed metadata and IIFE #Patch Set 4 : Use strict mode #
Total comments: 5
Patch Set 5 : Addressed comments #MessagesTotal messages: 7
New patch set: I fixed two corner cases where my implementation of Promise.all() failed. 2. The returned promise was never fulfilled when passing in an empty array. 1. Passing in non-promise values wasn't supported.
https://codereview.adblockplus.org/29333514/diff/29333620/lib/polyfill/promis... File lib/polyfill/promise.js (right): https://codereview.adblockplus.org/29333514/diff/29333620/lib/polyfill/promis... lib/polyfill/promise.js:18: "use strict"; For reference, I discussed strict mode with Wladimir. And we agreed to always enable strict mode for new scripts now.
This code is (necessarily) quite complex, I've tried my best with the review but I could well have missed something. https://codereview.adblockplus.org/29333514/diff/29333515/lib/polyfill/promis... File lib/polyfill/promise.js (right): https://codereview.adblockplus.org/29333514/diff/29333515/lib/polyfill/promis... lib/polyfill/promise.js:23: var PENDING = 0; Nit: const? https://codereview.adblockplus.org/29333514/diff/29333515/lib/polyfill/promis... lib/polyfill/promise.js:44: Promise.prototype = { Should be `global.Promise.prototype = {`? https://codereview.adblockplus.org/29333514/diff/29333515/lib/polyfill/promis... lib/polyfill/promise.js:76: if (this._state == REJECTED && this._subscriptions.length == 0) What if there are subscriptions but only for resolve, not reject? Couldn't we also have an uncaught rejection in that case? https://codereview.adblockplus.org/29333514/diff/29333515/lib/polyfill/promis... lib/polyfill/promise.js:101: setTimeout( How come you're using setTimeout here and above? https://codereview.adblockplus.org/29333514/diff/29333515/lib/polyfill/promis... lib/polyfill/promise.js:112: What about Promise.race? https://codereview.adblockplus.org/29333514/diff/29333515/lib/polyfill/promis... lib/polyfill/promise.js:115: if (value instanceof Promise) The docs say that we should use the value straight if it has a `.then` method, even if it's not an instance of Promise. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... https://codereview.adblockplus.org/29333514/diff/29333515/lib/polyfill/promis... lib/polyfill/promise.js:129: var count = promises.length; Nit: Call this `remaining` instead of `count`? https://codereview.adblockplus.org/29333514/diff/29333515/metadata.common File metadata.common (right): https://codereview.adblockplus.org/29333514/diff/29333515/metadata.common#new... metadata.common:4: lib/polyfill/promise.js Nit: Should call the directory polyfills? https://codereview.adblockplus.org/29333514/diff/29333620/lib/polyfill/promis... File lib/polyfill/promise.js (right): https://codereview.adblockplus.org/29333514/diff/29333620/lib/polyfill/promis... lib/polyfill/promise.js:18: "use strict"; On 2016/01/15 15:37:06, Sebastian Noack wrote: > For reference, I discussed strict mode with Wladimir. And we agreed to always > enable strict mode for new scripts now. Acknowledged.
https://codereview.adblockplus.org/29333514/diff/29333620/lib/polyfill/promis... File lib/polyfill/promise.js (right): https://codereview.adblockplus.org/29333514/diff/29333620/lib/polyfill/promis... lib/polyfill/promise.js:18: "use strict"; On 2016/01/15 19:00:42, kzar wrote: > On 2016/01/15 15:37:06, Sebastian Noack wrote: > > For reference, I discussed strict mode with Wladimir. And we agreed to always > > enable strict mode for new scripts now. > > Acknowledged. Shouldn't "use strict" be inside of the IIFE, otherwise strict mode will be turned on for all background scripts right?
https://codereview.adblockplus.org/29333514/diff/29333515/lib/polyfill/promis... File lib/polyfill/promise.js (right): https://codereview.adblockplus.org/29333514/diff/29333515/lib/polyfill/promis... lib/polyfill/promise.js:23: var PENDING = 0; On 2016/01/15 19:00:41, kzar wrote: > Nit: const? const isn't supported in strict mode before Chrome 41. It causes a SyntaxError in older versions. Even though it has been supported with non-standard semantics in non-strict mode since Chrome 21. https://codereview.adblockplus.org/29333514/diff/29333515/lib/polyfill/promis... lib/polyfill/promise.js:44: Promise.prototype = { On 2016/01/15 19:00:41, kzar wrote: > Should be `global.Promise.prototype = {`? As globals literally refers to the global scope, Promise is essentially the same as global.Promise. But let's cache Promise into in local variable if it makes the code less confusing. Also that way we avoid side-effects in the theoretical scenario when using Promise in a function that is called after Promise has been overridden in the global scope. https://codereview.adblockplus.org/29333514/diff/29333515/lib/polyfill/promis... lib/polyfill/promise.js:76: if (this._state == REJECTED && this._subscriptions.length == 0) On 2016/01/15 19:00:41, kzar wrote: > What if there are subscriptions but only for resolve, not reject? Couldn't we > also have an uncaught rejection in that case? No, in that case the error will be delegated to the next promise, the one returned by then(). In fact, if we would log the error here already we would eventually log it twice, or it would be logged even if it is handled later. For example with following code: fetch("http://example.com").then( response => response.text() ).then( text => { ... }, error => { alert(error); ] ); There is no onRejected callback for the promise returned by fetch(). So errors get delegated to the next promise in the chain, where an onRejected callback handles all errors that might have occurred up the chain. https://codereview.adblockplus.org/29333514/diff/29333515/lib/polyfill/promis... lib/polyfill/promise.js:101: setTimeout( On 2016/01/15 19:00:41, kzar wrote: > How come you're using setTimeout here and above? The callbacks are supposed to be called asynchronous. Otherwise calling code will experience inconsistent behavior dependent on whether resolve/reject is called synchronously or later. This is consistent with how native promises behave. Feel free to run following code with native promises on Chrome, Safari or Firefox: new Promise(function(resolve, reject) { resolve(1); console.log(2) }).then(x => console.log(x)); console.log(3); It will log: 2 3 1 https://codereview.adblockplus.org/29333514/diff/29333515/lib/polyfill/promis... lib/polyfill/promise.js:112: On 2016/01/15 19:00:41, kzar wrote: > What about Promise.race? I rather don't introduce dead code and unnecessary complexity. But just implement the APIs we eventually need. The same goes for the other polyfills. In particular for fetch(), the poliyfill would be way more complex if it would be complete. And the APIs we don't use would remain untested. https://codereview.adblockplus.org/29333514/diff/29333515/lib/polyfill/promis... lib/polyfill/promise.js:115: if (value instanceof Promise) On 2016/01/15 19:00:41, kzar wrote: > The docs say that we should use the value straight if it has a `.then` method, > even if it's not an instance of Promise. > > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... True, however this behavior isn't implemented in V8. And since we use native Promises on modern Chrome versions, I think it's more important to be consistent with the behavior there, in particular if the inconsistency would add additional complexity. https://codereview.adblockplus.org/29333514/diff/29333515/lib/polyfill/promis... lib/polyfill/promise.js:129: var count = promises.length; On 2016/01/15 19:00:41, kzar wrote: > Nit: Call this `remaining` instead of `count`? Since this variable is used as well to set the size of the resulting array and to check whether the array is empty before resolving the promises, I think count is more accurate. https://codereview.adblockplus.org/29333514/diff/29333515/metadata.common File metadata.common (right): https://codereview.adblockplus.org/29333514/diff/29333515/metadata.common#new... metadata.common:4: lib/polyfill/promise.js On 2016/01/15 19:00:41, kzar wrote: > Nit: Should call the directory polyfills? Fine with me. https://codereview.adblockplus.org/29333514/diff/29333620/lib/polyfill/promis... File lib/polyfill/promise.js (right): https://codereview.adblockplus.org/29333514/diff/29333620/lib/polyfill/promis... lib/polyfill/promise.js:18: "use strict"; On 2016/01/15 19:03:55, kzar wrote: > On 2016/01/15 19:00:42, kzar wrote: > > On 2016/01/15 15:37:06, Sebastian Noack wrote: > > > For reference, I discussed strict mode with Wladimir. And we agreed to > always > > > enable strict mode for new scripts now. > > > > Acknowledged. > > Shouldn't "use strict" be inside of the IIFE, otherwise strict mode will be > turned on for all background scripts right? Putting "use script" on the top-level will turn on strict mode for the whole file, but not for other files even if they share the same context. Though, if files get concatenated, multiple files become one file and any top-level "use strict" will enable strict mode for the whole resulting file. However, this script isn't concatenated with other files. Also the scripts that are processed with jsHydra are automatically wrapped into an IIFE, anyway.
LGTM https://codereview.adblockplus.org/29333514/diff/29333515/lib/polyfill/promis... File lib/polyfill/promise.js (right): https://codereview.adblockplus.org/29333514/diff/29333515/lib/polyfill/promis... lib/polyfill/promise.js:23: var PENDING = 0; On 2016/01/19 11:34:17, Sebastian Noack wrote: > On 2016/01/15 19:00:41, kzar wrote: > > Nit: const? > > const isn't supported in strict mode before Chrome 41. It causes a SyntaxError > in older versions. Even though it has been supported with non-standard semantics > in non-strict mode since Chrome 21. Acknowledged. https://codereview.adblockplus.org/29333514/diff/29333515/lib/polyfill/promis... lib/polyfill/promise.js:44: Promise.prototype = { On 2016/01/19 11:34:17, Sebastian Noack wrote: > On 2016/01/15 19:00:41, kzar wrote: > > Should be `global.Promise.prototype = {`? > > As globals literally refers to the global scope, Promise is essentially the same > as global.Promise. But let's cache Promise into in local variable if it makes > the code less confusing. Also that way we avoid side-effects in the theoretical > scenario when using Promise in a function that is called after Promise has been > overridden in the global scope. Acknowledged. https://codereview.adblockplus.org/29333514/diff/29333515/lib/polyfill/promis... lib/polyfill/promise.js:76: if (this._state == REJECTED && this._subscriptions.length == 0) On 2016/01/19 11:34:17, Sebastian Noack wrote: > On 2016/01/15 19:00:41, kzar wrote: > > What if there are subscriptions but only for resolve, not reject? Couldn't we > > also have an uncaught rejection in that case? > > No, in that case the error will be delegated to the next promise, the one > returned by then(). In fact, if we would log the error here already we would > eventually log it twice, or it would be logged even if it is handled later. > > For example with following code: > > fetch("http://example.com").then( > response => response.text() > ).then( > text => { ... }, > error => { alert(error); ] > ); > > There is no onRejected callback for the promise returned by fetch(). So errors > get delegated to the next promise in the chain, where an onRejected callback > handles all errors that might have occurred up the chain. Acknowledged. https://codereview.adblockplus.org/29333514/diff/29333515/lib/polyfill/promis... lib/polyfill/promise.js:101: setTimeout( On 2016/01/19 11:34:18, Sebastian Noack wrote: > On 2016/01/15 19:00:41, kzar wrote: > > How come you're using setTimeout here and above? > > The callbacks are supposed to be called asynchronous. Otherwise calling code > will experience inconsistent behavior dependent on whether resolve/reject is > called synchronously or later. > > This is consistent with how native promises behave. Feel free to run following > code with native promises on Chrome, Safari or Firefox: > > new Promise(function(resolve, reject) { > resolve(1); > console.log(2) > }).then(x => console.log(x)); > console.log(3); > > It will log: > > 2 > 3 > 1 Acknowledged. https://codereview.adblockplus.org/29333514/diff/29333515/lib/polyfill/promis... lib/polyfill/promise.js:112: On 2016/01/19 11:34:17, Sebastian Noack wrote: > On 2016/01/15 19:00:41, kzar wrote: > > What about Promise.race? > > I rather don't introduce dead code and unnecessary complexity. But just > implement the APIs we eventually need. The same goes for the other polyfills. In > particular for fetch(), the poliyfill would be way more complex if it would be > complete. And the APIs we don't use would remain untested. Acknowledged. https://codereview.adblockplus.org/29333514/diff/29333515/lib/polyfill/promis... lib/polyfill/promise.js:115: if (value instanceof Promise) On 2016/01/19 11:34:22, Sebastian Noack wrote: > On 2016/01/15 19:00:41, kzar wrote: > > The docs say that we should use the value straight if it has a `.then` method, > > even if it's not an instance of Promise. > > > > > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... > > True, however this behavior isn't implemented in V8. And since we use native > Promises on modern Chrome versions, I think it's more important to be consistent > with the behavior there, in particular if the inconsistency would add additional > complexity. Acknowledged. https://codereview.adblockplus.org/29333514/diff/29333620/lib/polyfill/promis... File lib/polyfill/promise.js (right): https://codereview.adblockplus.org/29333514/diff/29333620/lib/polyfill/promis... lib/polyfill/promise.js:18: "use strict"; On 2016/01/19 11:34:22, Sebastian Noack wrote: > On 2016/01/15 19:03:55, kzar wrote: > > On 2016/01/15 19:00:42, kzar wrote: > > > On 2016/01/15 15:37:06, Sebastian Noack wrote: > > > > For reference, I discussed strict mode with Wladimir. And we agreed to > > always > > > > enable strict mode for new scripts now. > > > > > > Acknowledged. > > > > Shouldn't "use strict" be inside of the IIFE, otherwise strict mode will be > > turned on for all background scripts right? > > Putting "use script" on the top-level will turn on strict mode for the whole > file, but not for other files even if they share the same context. > > Though, if files get concatenated, multiple files become one file and any > top-level "use strict" will enable strict mode for the whole resulting file. > However, this script isn't concatenated with other files. Also the scripts that > are processed with jsHydra are automatically wrapped into an IIFE, anyway. Acknowledged. 
|
