-
Notifications
You must be signed in to change notification settings - Fork 106
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
Support missing import.meta.env #413
base: main
Are you sure you want to change the base?
Conversation
Thanks for this PR. It seems like maybe the vite solution still has a kink to work out? vitejs/vite#8663. Maybe we should hold off until that issue is solved, before adding this. |
packages/builder-vite/envs.ts
Outdated
@@ -22,7 +22,7 @@ export const allowedEnvPrefix = ['VITE_', 'STORYBOOK_']; | |||
* Customized version of stringifyProcessEnvs from @storybook/core-common which | |||
* uses import.meta.env instead of process.env and checks for allowed variables. | |||
*/ | |||
export function stringifyProcessEnvs(raw: EnvsRaw, envPrefix: UserConfig['envPrefix']) { | |||
export function stringifyProcessEnvs(raw: EnvsRaw, envPrefix: UserConfig['envPrefix'], isBuild: boolean) { |
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.
Not sure what the preferred way to do this is, but happy to change it if needed.
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.
Let's use a mode
of either production
or development
, to avoid a boolean trap.
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.
I feel like mode
would be confusing here, because it's possible to build in development
mode and serve in production
. And vite
itself uses command
to derive this information:
https://github.com/vitejs/vite/blob/908c9e4cdd2cceb0f01495e38066ffe33c21ddb8/packages/vite/src/node/plugins/define.ts#L12
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.
Would you be okay with command: 'build' | 'serve'
as an argument?
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.
Sounds good, thanks.
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.
Done!
Happy to wait and see whether Vite team finds another approach to the problem, but it seems unlikely though. |
Vite 3 has been released, and vitejs/vite#8663 is not yet solved. I'm happy to proceed with this if the underlying problem is still an issue in Vite 3 (would you mind confirming?). |
Don't mind at all, but will only be able to do that in a week. |
So with Vite 3 and vitejs/vite#8663 not fixed, even this PR doesn't fix building storybooks ¯\_(ツ)_/¯ |
Currently vite storybook builder breaks when any env var used in code via
import.meta.env
is missing while building storybooks.Apparently that happens because Rollup does direct inline replacements and turns code like
into something like
which is syntactically incorrect.
See my reproduction repo for an example — https://github.com/princed/vite-sb-meta-env-repro.
To mitigate this
vite
itself adds a replacement fromimport.meta.env.
to({}).
to catch missing direct replacements and produce correct code. I opted for identical solution here.A similar workaround is also possible on the user side: