-
Notifications
You must be signed in to change notification settings - Fork 920
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: replace %SNOWPACK_PUBLIC_*%
and %MODE%
environment variables in HTML
#900
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pikapkg/snowpack/9nudk1pya |
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.
Looking good!
Can we add/clarify the docs at https://www.snowpack.dev/#environment-variables in this PR as well?
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.
all set
@@ -197,7 +197,7 @@ export async function buildFile( | |||
): Promise<SnowpackBuildMap> { | |||
// Pass 1: Find the first plugin to load this file, and return the result | |||
const loadResult = await runPipelineLoadStep(srcPath, buildFileOptions); | |||
// Pass 2: Pass that result through every plugin transfomr() method. | |||
// Pass 2: Pass that result through every plugin transform() method. |
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.
💯
ping @FredKSchott does that sound good?
hmr: false, | ||
config: this.config, | ||
mode: 'production', | ||
}); |
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 had to add the mode
option to differentiate between the build & dev commands
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.
LGTM!
return `export default ${JSON.stringify(envObject)};`; | ||
} | ||
|
||
const PUBLIC_ENV_REGEX = /^SNOWPACK_PUBLIC_/; |
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.
Should it be /^SNOWPACK_PUBLIC_.*/
so it doesn't match a SNOWPACK_PUBLIC_
variable?
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.
yea, good catch. I think that that's worth adding. technically it should be /^SNOWPACK_PUBLIC_.+/
, since * can match 0 items.
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.
Indeed, /^SNOWPACK_PUBLIC_.+/
is what I meant 😉
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'll make I made a follow up issue so we can ship this change: #924
%SNOWPACK_PUBLIC_*%
and %MODE%
environment variables in HTML
fixes #898 /cc @silverwind @FredKSchott
Changes
Testing
test/build/