-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
fix(remix-dev/vite): make installGlobals
opt-in for Vite dev
#8119
fix(remix-dev/vite): make installGlobals
opt-in for Vite dev
#8119
Conversation
🦋 Changeset detectedLatest commit: 6c783b7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
installGlobals
opt-in for Vite devinstallGlobals
for Vite dev
installGlobals
for Vite devinstallGlobals
for Vite dev
installGlobals
for Vite devinstallGlobals
for Vite dev
installGlobals
for Vite devinstallGlobals
opt-in for Vite dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! This approach seemed odd to me at first, but the more I thought about it, the more it made sense to me. When using Vite dev directly, the Vite config is the closest thing you have to an entry point, so calling it anywhere else (e.g. by passing a callback to the Remix plugin) feels like indirection.
🤖 Hello there, We just published version Thanks! |
🤖 Hello there, We just published version Thanks! |
I wrote this PR's rational in #8062 (comment)
I moved
installGlobals
totemplates/unstable-vite/vite.config.ts
, which looks a little ugly but the intention is to align withtemplates/unstable-vite-express
(and also other non-vite templates). If the time has come to removeinstallGlobals
, then that can be done by just by updating templates.It might be worth mentioning that currently
remix-serve
has automaticinstallGlobals
, so if users rely on defaultnpm run start
script, then they cannot avoid polyfills but I think it's same as other templates as well.Testing Strategy
CI runs only Node 18 and 20 (or maybe only on 20?), so I didn't update
vite.config.ts
of integration tests.But if it's still desired to test against
installGlobals
polyfills, then I think it might make sense to extra test case for that. Please let me know if that's preferred.