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

feat(remix-node): only polyfill globals if they're not present #6259

Conversation

MichaelDeBoey
Copy link
Member

@MichaelDeBoey MichaelDeBoey commented May 1, 2023

Closes #5858
Closes #6280
Closes #6284
Closes #6288
Closes #6290
Closes #6318
Closes #6348
Closes #6380

@changeset-bot
Copy link

changeset-bot bot commented May 1, 2023

⚠️ No Changeset found

Latest commit: 3618a82

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@MichaelDeBoey MichaelDeBoey force-pushed the conditionally-polyfill-Node-globals branch from 7fcc391 to b04638e Compare May 1, 2023 03:07
@isker
Copy link

isker commented May 1, 2023

I'm not sure it will be as easy as this. Before I gave up on using remix for my project, I had to patch other parts of it which were relying on polyfill implementation details in order to get rid of the polyfills: isker/neogrok@c47fc1c#diff-49b936c82e6771ef5a3b352fb8cc1fef5a732a395be7a2bb56ac508bf90e7f79.

(I didn't bother looking into what the semantics of raw in the polyfill were, which might influence how serious this change would be.)

@brophdawg11
Copy link
Contributor

I'm not sure if this is what we want since we've found that the node globals aren't spec compliant. I believe @jacob-ebey was working on a new way in which we give users control on whether or not to add polyfills.

@machour
Copy link
Collaborator

machour commented May 1, 2023

Also beware, ||= is node >= 15. We want to maintain node 14 compatibility.

@MichaelDeBoey
Copy link
Member Author

MichaelDeBoey commented May 1, 2023

Also beware, ||= is node >= 15. We want to maintain node 14 compatibility.

Code will be transpiled to Node 14, no? 🤔

@machour
Copy link
Collaborator

machour commented May 1, 2023

Code will be transpiled to Node 14, no? 🤔

You're correct, I totally missed #5047, thanks!

@bryce-pearce
Copy link

@brophdawg11 @MichaelDeBoey

Are there any updates on that other approach you mentioned? It seems like this is a breaking issue for several people

#6284
#6280
#6290

@MichaelDeBoey MichaelDeBoey force-pushed the conditionally-polyfill-Node-globals branch from 89c353d to 3618a82 Compare May 30, 2023 19:53
@MichaelDeBoey
Copy link
Member Author

Superseded by #6547

@MichaelDeBoey MichaelDeBoey deleted the conditionally-polyfill-Node-globals branch June 6, 2023 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants