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

Promise polyfill injects dead code in browser build tools #142

Closed
pjeby opened this issue Nov 14, 2014 · 4 comments
Closed

Promise polyfill injects dead code in browser build tools #142

pjeby opened this issue Nov 14, 2014 · 4 comments

Comments

@pjeby
Copy link

pjeby commented Nov 14, 2014

The current strategy of directly require-ing a promise polyfill causes build tools that parse require() statements to include the promise polyfill code in their build, unless specially configured to do otherwise. (Which in most such build tools, alas, is a non-trivial process.)

Common practice for use of polyfills in code that runs on both the browser and node, is to require the polyfill to be loaded by the library's user, rather than by the library, in order to avoid issues like this. Making polyfills the responsibility of the overall app, rather than the library, means that there is no functionality duplication in the final build.

So, it would be nice if regenerator would either drop the direct polyfill injection, or provide an option to specify a version of the runtime that doesn't even try to require() the polyfill in the source code.

Oh, and in case you're wondering why I care about this, specifically, it's because I'm the author/maintainer of regenerator-loader, which is the wrapper that lets people use regenerator with webpack -- a tool similar to browserify, but with more programmability.

I'm trying to update the module and its docs to work with the latest version of regenerator, but I've hit a snag in trying to ensure that people upgrading don't end up with an unnecessary promise polyfill in their project, especially if they're not using async functions in the first place!

At the moment, in order to work around it, I'd have to effectively embed the runtime in an eval(), (which I'd prefer to avoid, since the runtime would then not be minifiable). Or else I'd need to fork and embed regenerator itself, with all the consequent maintenance headaches.

So, all things considered, it would be helpful if regenerator either never tried to require() a polyfill, or at least had a variant runtime available that didn't.

For example, if there were a regenerator/node-runtime.js that checked for a promise implementation and required regenerator/runtime.js after loading the polyfill, then tools like webpack and browserify could use regenerator/runtime, even if the default code generation used regenerator/node-runtime.

I'll understand if your other design goals don't allow for this, but if your intended audience is browser developers rather than just node users, it would be generally helpful, and not just for webpack users. Browser developers generally understand the need to globally install the polyfills they need.

Also, If there's anything I can do to help implement this, please let me know. Thanks!

@benjamn
Copy link
Collaborator

benjamn commented Nov 14, 2014

I'm convinced! And, if anyone ever complains about Regenerator not working without an external Promise implementation, I can just point them to this issue.

@benjamn benjamn closed this as completed in d511556 Dec 2, 2014
benjamn added a commit that referenced this issue Dec 2, 2014
The minor version bump is just in case anyone was relying on the Promise
polyfill in runtime.js (see #142).
@amasad
Copy link
Contributor

amasad commented Dec 23, 2014

Given this, what's the best way to implement a global Promise in an app? It sucks that every file that is regenerated to have to var Promise = require("promise")

@dashed
Copy link

dashed commented Dec 23, 2014

@amasad I use webpack to provide Promise variable to be loaded with bluebird lib: http://webpack.github.io/docs/list-of-plugins.html#provideplugin

@pjeby
Copy link
Author

pjeby commented Dec 24, 2014

Given this, what's the best way to implement a global Promise in an app? It sucks that every file that is regenerated to have to var Promise = require("promise")

For the browser, use a polyfill that sets window.Promise. For node, have the main app set global.Promise.

(I would also actually suggest that the regenerator runtime refer to (regeneratorRuntime.Promise || Promise) so that things like gnode can install a Promise attribute on the runtime, rather than on global directly. That way, if you need regenerator to use an isolated promise implementation, you can.)

popomore added a commit to spmjs/spm-client that referenced this issue Jan 22, 2015
Promise is removed from regenerator!!! 💢

facebook/regenerator#142
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

No branches or pull requests

4 participants