Closed samselikoff closed 4 years ago
stefanpenner
commented
4 years ago It's likely ok to bundle whatwg-fetch as part of a dev time build step and presenting them as one bundle to consumers.
But I am not super enthusiastic about actually copying whatwg-fetch code & tests into this repo. It seems like keeping it simple to upgrade if we want to is important, and having the code copied seems like a hazard.
I can be convinced, these are only my first thoughts.
samselikoff
commented
4 years ago Ya. I think we would just copy this file: https://github.com/github/fetch/blob/master/fetch.js. So 500 lines of code. The last release of whatwg-fetch was 1 year ago.
If there were updates to whatwg-fetch, it's true we wouldn't know about them (at least right away). But again we should compare that to the current situation, in which we're relying on internals, which means whatwg-fetch could push out a patch version that refactors away from their reliance on XMLHTTPRequest and that would break Pretender. Is that likely to happen? Not sure. There are other newer polyfills of fetch that don't use XMLHTTPRequest, so maybe. In any case I do think it's strange for us to be relying on the internals of an external dependency.
I don't think we'd need whatwg-fetch's tests, I think our tests cover Pretender's public API and that's sufficient.
An alternative would be to update Pretender's code to "properly" support intercepting fetch, i.e. actually proxy the fetch APIs to Pretender's. But that would effectively involve bringing in a fetch implementation, and pointing it to our APIs, which effectively would boil down to something that looks a lot like bringing in whatwg-fetch and using it as the boundary layer to redirect fetch APIs to our own.
So ultimately I think vendoring whatwg-fetch is a lot like adding a "proper" fetch adapter to Pretender.
xg-wang
commented
4 years ago I haven't digged into how next.js resolves import * from 'whatwg-fetch', I would +1 to Stef's suggestion to do this at build time and make sure the scoped xhr is used.
Not sure about proxying fetch. It seems like building another polyfill, which whatwg-fetch already did the work.
samselikoff
commented
4 years ago Ohh I see – I don't think I fully understood.
Are you all saying we could keep whatwg-fetch as a dependency here in package.json, but for Pretender's publishing step, go ahead and pull it in + make it into an internal dependency? So there's no chance of it being overridden, hoisted or something else?
samselikoff
commented
4 years ago Another point @ryanto raised is that, currently Pretender is not importable in node, due to whatwg-fetch assuming a browser-like environment. This is another headache for us because even though Mirage doesn't yet work in Node, it needs to be importable so it doesn't break setups like Gatsby that expect isomorphic app code.
This led us needing to create some polyfills and use them like this: https://github.com/miragejs/miragejs/blob/23fb9fe3278af513e1a95c8a09c669e4a47fcd45/lib/server.js#L3. This ensure our polyfill code runs before Pretender is imported (and thus before whatwg-fetch is imported). The code essentially just ensures self is defined, which is all whatwg-fetch needs to be importable (again, not functional, which is not what we're interested at this time).
So, if we were to copy the 500-line whatwg-fetch file directly into Pretender, we could also solve this issue and make sure Pretender doesn't break node apps if it's in the import path, by simply wrapping the code in an if global.self === 'undefined') block.
I understand the purist argument about leaving whatwg-fetch in package.json, but Pretender's build file + logic is already pretty messy + hard to read/maintain, and whatwg-fetch's last release was over a year ago, so I think pragmatically the simplest way to solve this problem today is to just create a new file in Pretender, copy the 500 lines, make a comment at the top of the file and be done with it. And I also think the risk is very low.
Just some more of my thoughts!
stefanpenner
commented
4 years ago @samselikoff I don't have strong feelings either way, but am happy to support your calls here. They seem perfectly reasonable.
samselikoff
commented
4 years ago Right, thanks man. If we do pull the trigger on this, it will be quite easy to undo if it causes any issues.
Thanks a lot Stef!
GriffinSauce
commented
4 years ago Really appreciate the effort here guys! 🙌
samselikoff
commented
4 years ago Any trick to getting Pretender's test suite running locally? I started work on this but master is failing on my machine for a bunch of reasons...
GriffinSauce
commented
4 years ago test does run successfully after running test-ci once, seems like the pretest task is missing in the test command, does that help?
samselikoff
commented
4 years ago @GriffinSauce not sure but thanks for the suggestion, after Thomas' latest commit I have it working + can now get to work on this!
xg-wang
commented
4 years ago
Pretender uses the
whatwg-fetchpackage in order to interceptfetchcalls.Even though
whatwg-fetchis explicitly imported here (and used here), Pretender is actually relying implicitly onwhatwg-fetch's internals to get thefetchintercepting behavior working correctly.The reason is because the
fetchimplementation thatwhatwg-fetchprovides actually delegates towindow.XMLHTTPRequestunder the hood. Since Pretender also monkey patcheswindow.XMLHTTPRequest, it is able to interceptfetchcalls equally as well. But it's only due to those internals – due to the fact that thewhatwg-fetchpolyfill happens to use XMLHTTPRequest under the hood. This is not part ofwhatwg-fetch's public API, even though it is unlikely the change.The reason this bit us recently is because some folks are trying to use Mirage with Next.js. Next performs some build-time optimizations to ensure apps don't include multiple or heavy versions of popular polyfills,
whatwg-fetchbeing one of them. So, they actually swap out the package thatwhatwg-fetchresolves to, meaning when Pretender tries toimport * from 'whatwg-fetch', it ends up with a different polyfill entirely, and one that does not happen to useXMLHTTPRequestunder the hood. Therefore, Pretender (and thus Mirage) don't work.We asked about how to opt-out of this optimizing behavior here, but as of right now the Zeit team doesn't have any plans to make the behavior configurable.
In any case, regardless of whether Next.js chooses to expose such an option, it is still true that today, Pretender is relying on internals of
whatwg-fetch, and that poses a risk to Pretender in the event those internals ever change.I'm proposing that we vendor the latest version of
whatwg-fetchinto Pretender directly, since we are relying on observable but undocumented behavior of its current implementation. This would give us total control over the code, and would also address the Next.js issue sincewhatwg-fetchwould no longer be an external dependency that could be hoisted or replaced.