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

update polyfill with better native ES6 Promise test #99

Closed
wants to merge 1 commit into from

Conversation

lvscar
Copy link

@lvscar lvscar commented Apr 12, 2015

Object.prototype.toString.call(Promise.resolve()) === '[object Promise]'   

not works in Chrome

@stefanpenner
Copy link
Owner

Cc @jakearchibald


var nativeES6PromiseSupported =
P &&
// Some of these methods are missing from Firefox/Chrome experimental implementations
Copy link
Owner

Choose a reason for hiding this comment

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

i am unsure if we actually support those old/intermediate experimental version?

@stefanpenner
Copy link
Owner

not works in Chrome

yes but this is a chrome bug, but it is fixed in canary and should be available in stable soon:

screen shot 2015-04-22 at 3 30 38 am

As such, I don't believe we need this PR. As the existing check should cover these additional cases, and the buggy version of chrome without [object Promise] will age out soon thanks to its evergreen and auto-updating behaving.

@yethee
Copy link

yethee commented Apr 22, 2015

Why this condition !P.cast?

@piranna
Copy link

piranna commented May 11, 2015

As such, I don't believe we need this PR. As the existing check should cover these additional cases, and the buggy version of chrome without [object Promise] will age out soon thanks to its evergreen and auto-updating behaving.

You are right, but with current versions of both Chrome and Node.js (v0.12.2, latest stable) is not working (and the v8 update cycle of Node.js is not so fast as the one of Chrome), and also the checking mechanism of this pull-reques is more robust. I think this pull-request definitely needs to be merged.

@bradobro
Copy link

Seems to me P.resolve().constructor.name === "Promise" is a more resilient test in the current browser climate.

@stefanpenner
Copy link
Owner

As such, I don't believe we need this PR. As the existing check should cover these additional cases, and the buggy version of chrome without [object Promise] will age out soon thanks to its evergreen and auto-updating behaving.

@piranna
Copy link

piranna commented May 20, 2015

Why clossing this without merge? The current behaviour don't use native Promises on Node.js or Io.js.

@stefanpenner
Copy link
Owner

#99 (comment)

@piranna
Copy link

piranna commented May 20, 2015 via email

@stefanpenner
Copy link
Owner

Io.js will probably not update it to the
current version until at least 1 more year...

This doesn't seem correct, see: nodejs/node#1639 as such I believe iojs isn't nearly that far behind.

@piranna
Copy link

piranna commented May 20, 2015

Io.js will probably not update it to the
current version until at least 1 more year...

This doesn't seem correct, see: nodejs/node#1639 I believe iojs isn't nearly a full year behind.

But Node.js is...

@stefanpenner
Copy link
Owner

But Node.js is...

then those users can continue to use a fully functional polyfil, that is compliant (and significantly faster)

@piranna
Copy link

piranna commented May 20, 2015

But Node.js is...
then those users can continue to use a fully functional polyfil, that is compliant (and significantly faster)

For example? The only one fully compliant seems to be this one... I don't know why Node.js users need to use a polyfill having native ones... We are using this in production grade products...

@stefanpenner
Copy link
Owner

It has also come to my attention that v8 promises still have some bugs. (Recently discovered) So the strictness here is guarding us from those issues.

@dmitrykuznetsovdev
Copy link

Why this condition !P.cast?
safari not working.
wild brakes 👎 - D
Memory is growing very strongly

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

Successfully merging this pull request may close these issues.

6 participants