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

es6.promise.js feature testing causes console error in Chrome Canary v49.0.2615.0? #159

Closed
coopy opened this issue Jan 9, 2016 · 12 comments
Labels

Comments

@coopy
Copy link

coopy commented Jan 9, 2016

Uncaught (in promise) TypeError: promiseCapability.[[Resolve]] is not a function
    at Object.then (native)

I and some others are seeing this error pop up in React apps running in Chrome Canary. The theory is that this error is emanating from babel-polyfill, which is depending on core-js. See discussion at https://code.google.com/p/chromium/issues/detail?id=575314. Chromium seems to have enabled more strict checks.

Repro:

  1. git clone git@github.com:Ognian/test.git
  2. cd test && npm install && npm start
  3. Visit http://localhost:3002 in Chrome Canary v49.0.2615.0, with developer tools console open

Expected: no error
Actual: Error is displayed:

Uncaught (in promise) TypeError: promiseCapability.[[Resolve]] is not a function
    at Object.then (native)
@zloirock
Copy link
Owner

zloirock commented Jan 9, 2016

Thanks for the report. In Chrome 49, core-js currently uses native promises - looks like the bug is here. I will test it.

@zloirock
Copy link
Owner

zloirock commented Jan 9, 2016

var test = new Promise(function(){});
test.constructor = Object;
Promise.resolve(test);

is buggy in Chrome 49 native promises. And it creates problems for testing subclassing. Added fallback.

@zloirock zloirock added the v8 label Jan 9, 2016
@littledan
Copy link

What behavior are you observing, and what do you expect?

@zloirock
Copy link
Owner

zloirock commented Jan 9, 2016

@littledan subclassing. By the spec:

var test = new Promise(function(){});
Promise.resolve(test) === test; // should be `true`

var test = new Promise(function(){});
test.constructor = function(){};
Promise.resolve(test) === test; // should be `false`

But in Chrome 49 the second example creates rejected promise - bug. Why? Looks like .then uses .constructor instead of @@species.

@littledan
Copy link

It's true that we don't yet implement @@species (sorry, working on it!), but I don't think that's the cause here. For me, both of those tests pass. Which version of Chrome are you running? I tested the version of V8 that's in 49.0.2617.

@zloirock
Copy link
Owner

It's true that we don't yet implement @@species

The problem not in @@species implementation. SpeciesConstructor never returns just .constructor - it's buggy behavior.

but I don't think that's the cause here

It will not be caused here only with other serious bugs. I did not seek it in V8 source code - I just opened required method for confirming my guess.

If promise.constructor isnt Promise - Promise.resolve just something like new Promise(resolve => resolve(promise)) (not principal). Because promise is thenable, here will be called .then. .then creates NewPromiseCapability. By the spec - from SpeciesConstructor(promise, %Promise%), @@species missed -> just from Promise. But V8 uses this.constructor, tried to create NewPromiseCapability from bad constructor and does not have required [[Resolve]] and [[Reject]] functions. So that also will be rejected, but should not by the spec:

var test = new Promise(function(){});
test.constructor = function(){};
test.then(function(){});

For me, both of those tests pass.

Passed, but the second with unhandled rejection.

Which version of Chrome are you running? I tested the version of V8 that's in 49.0.2617.

49.0.2617.0 canary (64-bit)

Early 49 worked without unhandled rejection. 48- was completely replaced because of V8 bugs like 1, 2, etc.

@littledan
Copy link

Oh, I see what you mean--we currently take the constructor at face value, but really, we should only honor it if it has a @@species property (e.g., if it is a real subclass of Promise). To get that error on a spec-compliant Promises implementation, you'd have to add the line

test.constructor[Symbol.species] = test.constructor;

I was planning on shipping all of the @@species-related things as a package, ideally around M51, and leaving Promise subclassing in its current intermediate state for now, but it seems like this would cause issues for existing core.js users. At this point, I see three options:

  • Leave things how they are, and hope that users can live with this extraneous error message for just one release (while enjoying somewhat more correct Promises), and/or updating core.js to change the feature detection somewhat to avoid the issue.
  • Roll back some more of the Promise changes so that your existing feature detection will work and no error message is printed
  • Expedite a Promise-only addition of @@species support for Chrome 49 (while leaving other users of @@species out).

Which of these do you think would be reasonable?

@zloirock
Copy link
Owner

@littledan

The first option does not look like a good solution for me. Sure, it's not a critical problem - mainly error message, but serious. Fallback for that was added, but too many people use obsolete core-js versions. For example, babel-(polyfill|runtime) still uses ^1.2.x because of breaking changes in 2.0. Someone still uses 0.9.x with the same detection. For most people it's just an error message in console, but core-js has a basic support custom unhandled rejection handlers. Chrome 49 and Node.js - too. People who use it will have some problems, for example, with several babel-runtime instances.

From the second and the third options, the third seems to me more convenient and logical. Instead of, for example, Array methods, addition @@species support to Promise#then looks simple and should not cause any problems.

@littledan
Copy link

I'm starting with a rollback: https://codereview.chromium.org/1578893002 . This fixes the core.js console message in practice, but doesn't really leave things in an optimal state (like the third option does). I'm working on @@species in general, and we'll see if a backport ends up making sense (since we're about to branch for dev). Could you let me know if there are any issues with this revert, if it's an insufficient solution? Thanks for your help so far.

@littledan
Copy link

How well would core.js do if V8 shipped Symbol.species but didn't ship it on all types at once (namely not Array yet)?

@zloirock
Copy link
Owner

@littledan as I wrote - perfectly, without any problem :)

@JiangJie
Copy link

It disappeared in Google Chrome 50.0.2630.1 canary SyzyASan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants