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

Question about next.js and fetch polyfill #10509

Closed
ryanto opened this issue Feb 12, 2020 · 13 comments
Closed

Question about next.js and fetch polyfill #10509

ryanto opened this issue Feb 12, 2020 · 13 comments
Assignees

Comments

@ryanto
Copy link

ryanto commented Feb 12, 2020

Hello!

I have a dependency in my application that specifically relies on window.fetch being replaced with the whatwg-fetch polyfill. whatwg-fetch is a transitive dependency, so it should be ending up in my build, but it looks like next.js is replacing it with something else.

From reading the docs it seems like next provides it's own fetch polyfill? Does that sound right, or am I totally off track here?

Thanks!

@merelinguist
Copy link
Contributor

Yes, Next has polyfilled ‘fetch’ since 9.1.7. Hope that helps!

@ryanto
Copy link
Author

ryanto commented Feb 12, 2020

Ok cool.

Is there a way to make sure the whatwg-fetch polyfill from my dependency gets loaded?

@Timer
Copy link
Member

Timer commented Feb 12, 2020

No, it is not possible to load whatwg-fetch — we stub out all instances so your bundle is not bloated with unnecessary code duplication.

To reiterate: Next.js guarantees window.fetch is defined/polyfilled.
Your dependencies can still safely import whatwg-fetch and we'll ignore it and return window.fetch instead.

@Timer Timer closed this as completed Feb 12, 2020
@ryanto
Copy link
Author

ryanto commented Feb 13, 2020

Thanks for the quick answer. I appreciate that next is watching out for me and making sure my bundle is not bloated. It's pretty cool how next takes care of polyfilling these things for me.

Unfortunately my dependency depends on some of the specific implementation details of whatwg-fetch, so me not having access to that module puts me in tough spot. I was able to get whatwg-fetch loaded by using an npm/yarn alias.

yarn add original-whatwg-fetch@npm:whatwg-fetch

And then importing it like so...

import { fetch as originalWhatWgFetch } from 'original-whatwg-fetch';

@Timer
Copy link
Member

Timer commented Feb 13, 2020

Could you please show us what internals you're depending on? We should be able to make it work.

Code shouldn't be able to tell the difference.

@samselikoff
Copy link

We use Pretender.js which uses whatwg-fetch to polyfill fetch. whatwg-fetch works by proxying fetch calls to XMLHTTPRequest, which Pretender itself then polyfills to implement its interceptor behavior. So, Pretender implicitly relies on the specific way that whatwg-fetch polyfills fetch, in order to support intercepting fetch requests.

For this reason, whatwg-fetch is really more a dependency of our project, rather than a polyfill.

@Timer
Copy link
Member

Timer commented Feb 13, 2020

Aaah, in that case, yeah, we cannot support this out of the box. Thanks for the explanation!

@Timer Timer closed this as completed Feb 13, 2020
@samselikoff
Copy link

Hm, ok. I understand it's a bit of a strange use case, but it also seems like if Next is going to swap out a particular subset of dependencies, there should be a way to opt out of that...

If a Next project needed to add whatwg-fetch as a dependency, for whatever reason (or any of the other whitelisted polyfill dependencies), it seems like it's basically impossible to do so. That definitely seems like it could cause problems in certain situations.

How about a way to opt out of, or circumvent this behavior?

@Timer
Copy link
Member

Timer commented Feb 13, 2020

Relying on internals of any package is always dangerous, so I don't believe that's something we want to make easy.

If you absolutely require the internal implementation of this package, I'd suggest to keep using your workaround which adds 3kB gzip to your production bundle.

The best solution here seems to be Pretender.js adding support for window.fetch.

@samselikoff
Copy link

Sure, of course it is, but it's not our code. From our package's perspective, it's behavior we rely on in a third-party dependency. It doesn't make our code rely on internals – it makes the dependency work as intended.

Next is making an assumption here about the observable behavior of a number of dependencies, including whatwg-fetch, and choosing to prune/swap them out, as long as they fulfill that behavior. I understand why and I think it's good default behavior. But here we rely on a dependency that relies on different observable behavior of whatwg-fetch, so Next's swapping is breaking expected behavior from our package's perspective.

(FWIW our project is Mirage and folks are trying to get it working with Next.)

Again I think the default behavior is fine, but I think there should be an escape hatch, given Next is such a low-level general purpose tool, and given that this module-swapping behavior relies on the assumption that these specific dependencies are only ever included in Next apps for a certain set of their observable behavior (the public API) and not other observable behavior, which is not true in this case.

@Timer
Copy link
Member

Timer commented Feb 13, 2020

Allowing this to be configurable will open up the door for many users to deoptimize their applications unintentionally.

If Mirage requires a very specific implementation of window.fetch, it should vendor the package and override the global via self.fetch = vendoredWhatwgFetch.
This would also make Mirage more robust to upstream changes in whatwg-fetch, so it's a win-win.

Over 5000 packages depend on whatwg-fetch, so this optimization is pretty impactful.

@samselikoff
Copy link

I'm not saying the optimization is not good! I'm just saying that Pretender explicitly added it as a dependency, and even explicitly imports from it here: https://github.com/pretenderjs/pretender/blob/master/src/index.ts#L2

It's not that Mirage requires a specific implementation of window.fetch, it's that Pretender literally depends on the actual whatwg-fetch dependency. The developers added it and imported it because they wanted specific behavior from that specific package – which should be a completely valid thing for anyone to do, for any package + for any reason!

I do not think you should change the defaults, I'm just saying, swapping out a dependency like this is extra/magical behavior with a lot of assumptions baked into it. Doing it is fine, but not providing an escape hatch seems completely wrong to me. And I would bet good money that many users will not deoptimize their apps unintentionally... the escape hatch should be an advanced configuration option that folks in situations like ours opt-into in very specific situations.

@balazsorban44
Copy link
Member

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 30, 2022
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

5 participants