-
Notifications
You must be signed in to change notification settings - Fork 1
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
bump vite to 6.0.0-beta.8
#50
Conversation
@@ -140,28 +140,6 @@ export function createCloudflareEnvironmentOptions( | |||
createEnvironment(name, config) { | |||
return new CloudflareDevEnvironment(name, config); | |||
}, | |||
optimizeDeps: { |
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.
Vite's moved this option to the top level: vitejs/vite#18465
f66c929
to
c0ac005
Compare
@@ -178,7 +156,29 @@ export function createCloudflareEnvironmentOptions( | |||
external: [...cloudflareBuiltInModules], | |||
}, | |||
}, | |||
webCompatible: true, |
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.
They've also removed webCompatible
and kind of replaced it with keepProcessEnv
(vitejs/vite#18395 (comment))
I think it's best to hold off upgrading until #49 is merged. |
c0ac005
to
cd6d5fa
Compare
// We want to use `workerd` package exports if available (e.g. for postgres). | ||
conditions: ['workerd'], |
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.
@petebacondarwin I needed to move this to the environment config since conditions
is no longer inherited by environments (source change) I hope this is fine, was there a particular reason why you added the conditions
here?
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.
This was the place I found that worked. But if it works in the environment config that is also cool.
Does that cover builds too?
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.
Does that cover builds too?
It is a top-level configuration so I would definitely say yes 🤔
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.
mh.... but it seems like we have other build
issues.... I'll bring them up in chat
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.
Subject to fixing up Vite platform
issue.
Actually the CI jobs are not happy at all... |
yes, things only pass with my local version of vite (unless I do apply a pnpm patch) |
… add missing defaults)
6b077cc
to
debe5e0
Compare
3463e5c
to
9199a52
Compare
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.
Looks good to me!
Warning
This PR is blocked by this issue: https://github.com/vitejs/vite/pull/18395/files#r1832458011
Fixed by: vitejs/vite#18611 (this will apply only in the next Vite beta 10 release though)