sst / ion

SST v3
https://sst.dev
MIT License
1.89k stars 221 forks source link

Remix polyfill breaks certain streams #759

Open klaemo opened 2 months ago

klaemo commented 2 months ago

The installGlobals() polyfill is causing problems with newer remix version and certain http requests. Especially related to streams. Remix suggests that it's not necessary for node versions >=20. I wonder what the best strategy is to also support older remix versions with SST. Currently, I patched the polyfill to installGlobals({ nativeFetch: true }) to make it work. Maybe that's the way forward until it can be removed completely?

Bildschirmfoto 2024-07-25 um 18 40 52
jayair commented 2 months ago

This is for a deployed Remix site?

klaemo commented 2 months ago

Yeah! So, here's my understanding of installGlobals:

See remix's changelog: https://github.com/remix-run/remix/blob/main/CHANGELOG.md#undici

My suggestion is to switch to installGlobals({ nativeFetch: true }) and document it somewhere. And when Lambda gets node 22 in the future it should be safe to drop the polyfill altogether.

Or maybe even add a flag to let users control the polyfill behavior?!

fwang commented 1 month ago

@klaemo just to clarify, does changing installGlobals() to installGlobals({ nativeFetch: true }) break for remix versions prior to v2.9?

If installGlobals({ nativeFetch: true }) works for everyone (node 18/20, remix prior/after v2.9), we probably don't need to expose the polyfill behavior to user?

klaemo commented 1 month ago

just to clarify, does changing installGlobals() to installGlobals({ nativeFetch: true }) break for remix versions prior to v2.9?

It shouldn't, since that flag doesn't exist at all in those versions.

If installGlobals({ nativeFetch: true }) works for everyone (node 18/20, remix prior/after v2.9), we probably don't need to expose the polyfill behavior to user?

I agree that it's probably unnecessary to expose this to the user.