-
Notifications
You must be signed in to change notification settings - Fork 525
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(rollup): replace globalThis.process.
with process.
#1360
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1360 +/- ##
=======================================
Coverage 76.58% 76.59%
=======================================
Files 71 71
Lines 7223 7224 +1
Branches 718 718
=======================================
+ Hits 5532 5533 +1
Misses 1690 1690
Partials 1 1
|
@pi0
However, I'm not sure if that would introduce any breaking change. So maybe this could be done in another PR as a feat, and we could also extend the nitro config to allow custom options for maximum flexibility. https://github.com/rollup/plugins/tree/master/packages/replace#delimiters Note that I also opened an upstream issue with rollup directly rollup/plugins#1524 |
This one might still be useful - my suggestion would only prevent |
I tried the delimiters option alone, and it does fix this specific issue (ie |
Co-authored-by: pooya parsa <[email protected]>
globalThis.process.
with process.
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.
Thanks!
π Linked issue
fix: #1346
fix: nuxt/nuxt#21768
see vite PR logic: vitejs/vite#12194
β Type of change
π Description
Some packages uses
globalThis.process.env.NODE_ENV
instead ofprocess.env.NODE_ENV
, which isn't supported by default by rollup. Vite supports this and so does Nuxt, so this changes brings the exact same feature to the rollup config for Nitro.See issue in graphql-js
π Checklist