-
Notifications
You must be signed in to change notification settings - Fork 32
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
Why does this package pull in core-js? #524
Comments
The duplication is a bit unfortunate, yeah. We pull in core-js because we use the runtime transform, and we use the runtime transform because this pulls in a (pure) polyfill for async iterators, which we use for the resolver. I'm not aware of a better way to package this such that it works out-of-the-box for apps targeting older browsers, without forcing those apps to pull in their own polyfills. |
But if you can think of a better way to package this, I'd be happy to not pull in these runtime polyfills. |
I agree that for making it work out-of-the-box, there doesn't seem to be another way. Is supporting that behavior that a strict requirement for the project? My personal belief is that downstream apps should be responsible for polyfills and that libraries should alert devs to the potential need for polyfills in the docs. That's also how Relay and React do it. I don't think that it's ideal to do it that way, but IMHO that's the least surprising behavior. When making a web app, you always have to think about browser support and polyfills are generally part of the planning process. I don't think of polyfills as normal dependencies. The fact that they're needed at all is kinda a hack that we're forced to live with because of the shortcomings of the web as a platform. If we were building Linux libraries and desktop apps, for example, we'd expect the package manager and the package maintainers to ensure that everything uses the one blessed version of core-js. In Windows, static bundling is much more common, but then again package size is not critical on desktops the way that it is on the web. Anyway, my point is that given that polyfills are a kludge to begin with, I think it's right for the app devs to make the call over which ones are included. Even if one doesn't see them as a hack, it's arguably just as important to enable a dev to make their own choices about which browsers not to support as it is to make it easy for them to support older browsers. IMHO, alerting them to the potential need for polyfills in the docs without forcing them to use one strikes the right balance, and it's an expectation that's consistent with what they'd already be familiar with if they're supporting Relay on older browsers (see above). My $0.02. |
I think the distinction here is that async iterators are a bit more exotic than the kinds of polyfills that React and Relay tend to use. Saying "you need to have a |
You're right, the async iterator polyfill is more obscure and would probably cause a few web searches. But would it be any different in terms of documentation? You'd simply add |
I'm sorry to bug you about this again, but are you sure there isn't any way to drop the core-js-pure dependency? It's kind of crazy that this one package is forcing a 14 KB-gzipped and minified--polyfill in a bundle. It's the fourth largest dependency in my app, and that's not counting found-relay itself. I understand the desire to provide a better OOBE, but I firmly believe that libraries shouldn't force polyfills on end users. |
We'd be happy for this to be more minimal! Historically this has been hard tho, polyfilling an built-in symbol can be tricky b/c of how Babel is also annoying in that you can't pick and choose (that I know of) which things it should expect native implementations for and which things to pull a polyfill in. CoreJS 3 is also super aggressive about which things it pulls in, even when you cherry-pick only the things you want. So you get annoying things like polyfill fills for We should fix this tho, and there is already some work underway by @manon-pilaud to update the babel targets to something more modern and minimal. It might also be worth trying to use a specifically scoped async iterator transform if such a thing exists |
@jquense aha, I didn't realize that there's a deep history behind this particular polyfill. That makes sense. I had assumed that it would be trivial to simply do a
I believe that I'm misunderstanding you, but Babel is agnostic as to what you polyfill and what you don't (assuming that you configure
Any feature not on that list is implicitly native--except for the features polyfilled by found-react! 🙂 |
We might be able to use babel/babel#3934 (comment) to omit some of the polyfills. |
Just curious why the distributed code has references to core-js. Wouldn't it be better to let end users polyfill as needed?
Specifically, found-relay is causing core-js-pure to get pulled in, which ends up duplicating a lot of the core-js functions that normally get polyfilled (and pollute global namespace).
The text was updated successfully, but these errors were encountered: