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

babel-polyfill replaces existing Promise implementation #510

Closed
bisubus opened this issue Feb 5, 2017 · 4 comments
Closed

babel-polyfill replaces existing Promise implementation #510

bisubus opened this issue Feb 5, 2017 · 4 comments
Milestone

Comments

@bisubus
Copy link

bisubus commented Feb 5, 2017

This is caused by the fact that core-js/shim (which is used by babel-polyfill) aggressively applies polyfills to global objects if it thinks that they aren't good enough, almost every non-native Promise falls into this category.

Steps to reproduce

const NativePromise = global.Promise;
const Bluebird = global.Promise = require('bluebird');
require('feathers');
console.log(Promise !== NativePromise, Promise !== Bluebird);

Expected behavior

require('feathers') keeps globals intact. If there is another Promise implementation (i.e Bluebird which was installed globally as global.Promise), it isn't replaced with a polyfill.

If Promise or other JS feature should exist in supported Node versions but is missing in user Node version, the user is responsible for providing polyfills for them (falls into 'unsupported' category).

Actual behavior

require('feathers') loads babel-polyfill which replaces existing non-native Promise implementation with a polyfill.

System configuration

Tell us about the applicable parts of your setup.

Module versions (especially the part that's not working): [email protected]

NodeJS version: 7.2.0 x64

Operating System: Windows 7

@daffl
Copy link
Member

daffl commented Feb 7, 2017

I would assume that

require('feathers');
const Bluebird = global.Promise = require('bluebird');

works right? Could this be a bug with babel-polyfill in general?

I agree that core shouldn't include babel-polyfill automatically in the next breaking version though.

@bisubus
Copy link
Author

bisubus commented Feb 7, 2017

@daffl I' have global.Promise = require('bluebird') assigned in polyfills.js, which is obviously loaded before any other modules. I was taken by surprise that I've had to do this again after require('feathers'), this smells a bit. I've ended up with nonwritable descriptor for global.Promise (suck it up, core-js).

It depends on what the polyfill is used for in Feathers. For the features that Node 4 doesn't support it would probably be better to have fine-grained core-js polyfills, and for ES5 features the users are on their own. This lowers the footprint at least.

Not sure if this is a fault of babel-polyfill,

If you are looking for something that won't modify globals to be used in a tool/library, checkout the transform-runtime plugin. This means you won't be able to use the instance methods mentioned above like Array.prototype.includes.

transform-runtime will likely force Feathers to use local polyfilled Promise. If the library uses Promise indeed, it may need something like feathers.Promise property to allow users to replace internal promise implementation with their own (like Mongoose does, for example).

@daffl
Copy link
Member

daffl commented Sep 1, 2017

This has been fixed via #662 in v2.2.0

@lock
Copy link

lock bot commented Feb 7, 2019

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue with a link to this issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Feb 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants