-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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: $env/*/private
throws for client-side imports on Windows and Tailwind doesn't freak out
#5739
Conversation
🦋 Changeset detectedLatest commit: 00fbfeb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
is it possible to use vite's normalizePath here? https://vitejs.dev/guide/api-plugin.html#path-normalization |
I was hoping someone more knowledgeable would come along and make my day. |
I'm surprised none of the browser tests failed on Windows. Maybe we need one that uses |
The problem is that these are buildtime/dev-time errors, meaning we'd need a test project dedicated to attempting to build and ensuring the build fails. There are a number of things that'd be useful for -- I'm happy to put an implementation together, but probably not for this PR? |
I'm not sure you'd need anything too special or a separate test app. If you fail the build on one of the existing test apps it should turn the CI red |
Yes, but we're trying to verify that the build does fail, and the build needs to not fail in order for the rest of the tests to be able to run. Hence, the "metaproject" that tries to build purposefully-failing projects and verifies that they actually do fail. |
Oh, right. Yeah, that'd be kind of a pain to test |
Rich, there's another env var bug I want to work on tonight, but it might step on this one's toes. Easier to merge this one first, if you think it looks good. |
$env/*/private
throws for client-side imports on Windows$env/*/private
throws for client-side imports on Windows and Tailwind doesn't freak out
Heads up @Rich-Harris -- I went ahead and fixed the other bug in this PR as well. It was a pretty simple change. |
Okay, once we see the green christmas tree list we should be GTM @Rich-Harris |
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.
we need a snake_case lint rule!
actually i rescind my approval until i'm confident that the firefox tests are just flaking |
Closes #5723 and closes #5747.
The paths for both Vite and Rollup's module graphs needed to be normalized, along with the illegal import paths.
Because Tailwind needs to analyze basically every file in your project, Vite thinks it imports all of them, meaning it'll almost always import
$env/*/private
, causing our helpful module analysis to be, well, unhelpful. In dev, we now ignore non-js|ts|svelte
files. Rollup doesn't have this problem (because it doesn't have to worry about HMR), so we can be stricter and just analyze all imports. I think this strikes a happy medium. "Provide armor against footguns in dev, and provide even better armor when we try to go to prod."Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. All changesets should bepatch
until SvelteKit 1.0