Closed brettdh closed 8 months ago
LGTM, thank you for the PR! Also nice that remix removed the polyfills.
They are still needed in v1.19.3
so I'll publish this under a pre major release so it would be available already.
I'll do some testing when I have a little more time and release it under the major release later (v2.0.0
) 🙏
My intent was that a single version of remix-aws would continue to work with Remix v1 or v2; IIRC the globals are still available in Remix v1 without an explicit import. Did you find that not to be the case? (I did not test with Remix v1.)
As long as you use node 18 they are available but remix v1.19.3
supports node 14 which needs the globals installed and I vaguely remember having issues in node 16 when creating this project 🤔
I think it was here 👉 https://github.com/remix-run/remix/blob/remix%401.19.3/packages/remix-node/globals.ts#L45-L62
export function installGlobals() {
global.File = NodeFile;
global.Headers = NodeHeaders as typeof Headers;
global.Request = NodeRequest as typeof Request;
global.Response = NodeResponse as unknown as typeof Response;
global.fetch = nodeFetch as typeof fetch;
...
}
Headers
, Request
, Response
, fetch
, ... would not exist if you're using node 16 or 14. Which is why I kept using NodeHeaders
, NodeRequest
, NodeResponse
,...
pre release published
thanks again 🙏
But a project on Remix v1 could still use installGlobals
if they wanted to use this package, just as they must now do with Remix v2, right?
Feel free to release however you like; just trying to understand as I come up to speed on details of the Remix ecosystem. 🙂
This was bothering me so I had to test it now 😅
I ran a test on remix v1.19.3
on node 16 and it indeed works fine without any issues.
I did had issues when I was working on remix-aws running on node 16 and having the types of node 16 (@node/types@^16.6.2) so I was confusing the two.
If you're interested, I created a branch to illustrate the issue I ran into 👉 https://github.com/wingleung/remix-aws/compare/main...analysis-globals-issue-from-the-past
remix-aws itself has a requirement of using node 18 or up so it shouldn't be problem anymore https://github.com/wingleung/remix-aws/blob/main/package.json#L36-L38
I will still do a release on v1.0.0
just so we're out of the v0
stage
thanks for bearing with me 🙏
Thanks for the details and for the quick turnaround on this!
Tweaks required to handle these breaking changes in Remix v2:
Without these changes, I got this error: https://github.com/remix-run/remix/pull/7779#issuecomment-1783272478