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

Don't load core-js promise into global namespace #951

Closed
jason0x43 opened this issue Sep 24, 2018 · 6 comments
Closed

Don't load core-js promise into global namespace #951

jason0x43 opened this issue Sep 24, 2018 · 6 comments
Labels
bug Something that's not working as intended

Comments

@jason0x43
Copy link
Member

Intern 4.3.1 is failing some Promise shim tests in @dojo/framework on FF, Safari, and Edge, but not on Node, IE, and Chrome. It appears that the core-js shim is being loaded on those platforms even though native promises are available, and it's either behaving incorrectly or causing issues with the @dojo shim.

Have Intern import Promise from core-js rather than using the shim, or use a better shim (promise-polyfill doesn't appear to have the same issue).

@jason0x43 jason0x43 added the bug Something that's not working as intended label Sep 24, 2018
@jason0x43
Copy link
Member Author

For reference, apparently the issue is that core-js is particularly aggressive about when to apply its polyfill, overriding native Promise implementations when this may not be desirable (zloirock/core-js#283).

jason0x43 added a commit that referenced this issue Oct 21, 2018
The corejs polyfill was causing issues with @dojo/framework. Replace it
with promise-polyfill. Eventually, don't use a polyfill.

fixes #951
@jason0x43
Copy link
Member Author

promise-polyfill apparently isn't a great choice either -- it polyfills finally separately from it's standard Promise polyfill and can cause timing issues (such as finally resolving asynchronously for a canceled Task).

@jason0x43 jason0x43 reopened this Oct 22, 2018
jason0x43 added a commit that referenced this issue Oct 22, 2018
promise-polyfill's `finally` polyfill can causing timing issues for
finally handlers for cancelled Tasks.

resolves #951
@gitgrimbo
Copy link
Contributor

gitgrimbo commented Oct 22, 2018

FYI @jason0x43 - I had some problems with es6-promise and polyfilling for Edge. Not sure if this impacts your work.

stefanpenner/es6-promise#330 (comment)

and

https://stackoverflow.com/questions/50096031/typeerror-object-doesnt-support-property-or-method-finally

@jason0x43
Copy link
Member Author

I noticed some issues with es6-promise as well. All of these promise polyfills seem to have subtle but annoying implementation differences.

@jason0x43
Copy link
Member Author

Actually, it seems the latest issue isn't so much with es6-promise as it is with some weird native Promise behavior in older versions of Firefox (< 35). Intern's polyfilling just needs to install a basically functional Promise implementation, so the issue with es6-promise not polyfilling finally shouldn't be a problem. Really, though, Intern would be better off skipping the whole polyfill business; it should import a Promise for itself (that could use the native version or a shim) and not pollute the global namespace.

@jason0x43
Copy link
Member Author

Unfortunately that might end up being a significant change for downstream users, so...polyfilling for now.

jason0x43 added a commit that referenced this issue Oct 24, 2018
Use core-js, but be a bit more careful. Manually load polyfills only if
no native version exists. Generally Don't try to shim functionality into
existing native classes.

resolves #951
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that's not working as intended
Projects
None yet
Development

No branches or pull requests

2 participants