Inline process.env variable references directly when installing. #1044
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pikapkg/snowpack/98kpdj1su |
pkaminski
force-pushed
the
inline-process-env
branch
from
d011c8d to
bfff9ca
Compare
snowpack/src/commands/install.ts
Outdated
| rollup: userDefinedRollup, | ||
| treeshake: isTreeshake, | ||
| polyfillNode, | ||
| }, | ||
| } = config; | ||
|
|
||
| const env = { |
There was a problem hiding this comment.
So we actually have some great code here that you could bring back from older version of Snowpack, where this was the main way that we did process.env replacement. For example, I'd love to see this function brought back (but with everything besides env removed: https://github.com/pikapkg/snowpack/blob/v2.6/src/commands/install.ts#L127
That way, your plugin usage is simplified to:
rollupPluginReplace(getRollupReplaceKeys(userEnv)),
There was a problem hiding this comment.
I don't think we want to quite do that. getRollupReplaceKeys conflates computing the env dictionary and stringifying it for replacement. In my approach, I compute env first so that both the replacement and the process polyfill can use it, guaranteeing that they'll have the same process.env values.
However, my code is almost exactly the same -- it's just split into two parts. I changed it to use JSON.stringify instead, and I can break out the two reduces into functions if you'd like but I think they're so small it won't really improve readability much.
There was a problem hiding this comment.
Okay, that makes sense. The install() function is large enough as it is, so In that case it would still be great to move this specific chunk of logic into a separate const env = generateEnvObject(userEnv) function.
(the second reduce below is fine to keep inline)
There was a problem hiding this comment.
No problem, extracted. But I work from email so I didn't see your edit about keeping the second reduce inline and extracted it as well -- hope it's OK. 😊
| @@ -4631,6 +4631,11 @@ exports[`snowpack build package-tippy-js: web_modules/import-map.json 1`] = ` | |||
|
|
|||
| exports[`snowpack build package-tippy-js: web_modules/tippyjs.js 1`] = ` | |||
| "import { c as createPopper } from './common/popper-XXXXXXXX.js'; | |||
| /**! | |||
| * tippy.js v6.2.6 | |||
There was a problem hiding this comment.
huh, this is unexpected. Not necessarily bad, just suprising
There was a problem hiding this comment.
Yeah, I wasn't too sure what to make of it either. My only guess is that something (?) now knows statically that we're not making a production build, so it's keeping the comment there.
|
perfect timing too, I think this may be related to #1026 (reply in thread) Update: yup, I can confirm that this fixes #1026 for me 🎊 |
Changes
Inline process.env property references directly when installing, as discussed in #1000.
Testing
Existing tests seem to provide sufficient coverage for this change.
I assume the react-related changes in the snapshots are due to modules getting tree-shaken away thanks to inlining
NODE_ENV.This change is