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
avoid using process.env
with globalThis
#3927
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for compassionate-pike-271cb3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Hi @B2o5T, I'm @github-actions bot happy to help you with this PR 👋 Supported commandsPlease post this commands in separate comments and only one per comment:
|
The problem using You should be able to configure |
this
no need to manually configure webpack, everything should works fine out of box, never saw somebody uses |
@B2o5T If you fix up the logic that was incorrect in your code above, you will end up with code that will not be tree-shaken out if you do not replace the typeof process !== 'undefined' && process.env.NODE_ENV === 'production' cannot be tree-shaken, as it is essentially if (runtimeValue && true) {
//...
} else {
//...
} which comes down to if (runtimeValue) {
//...
} else {
//...
} It would only tree-shake away in development, never in production. That's why you need to configure your bundler to also replace the On the other hand, you can easily configure your current |
my code Current
|
I don't see the logic, please take a look of my fix that uses different code typeof process === 'undefined' || process.env.NODE_ENV === 'production' CAN be tree-shaken, if (runtimeValue || true) {
//...
} else {
//...
} which comes down to - always truthy if (runtimeValue || true) {
//...
} |
As I already said multiple times, that is a completely different logic than before your change. It will not give dev warnings if
That is not the current code, you are operating on an outdated branch. |
it's a code from the graphql-js/src/jsutils/instanceOf.ts Line 12 in d766c8e
or https://github.com/graphql/graphql-js/blob/main/src/jsutils/instanceOf.ts#L12 |
For reference: This is the code before graphql-js/src/jsutils/instanceOf.ts Lines 9 to 18 in 0926554
|
Because we are talking about Version 16 here. The main branch targets v17 and is currently up to date with this change from 16.7.0, but not yet with 16.7.1: https://github.com/graphql/graphql-js/blob/16.x.x/src/jsutils/instanceOf.ts#L12 |
if typeof process === 'undefined' || process.env.NODE_ENV === 'production' false || false will give dev warnings, or you talking about non node environment without |
@phryneas Ok, I reread your suggestions, #3927 (comment) you can apply it |
Anything like that. If a user wanted to use this in ESM and disable dev warnings, they could still manually declare a global variable to turn them off. In the end, I am not a maintainer of this lib and not the one you need to convince of a final change, but please be transparent what you are introducing here - and this one changes the current intent of the prod/dev check, disguised as just a bugfix. |
Co-authored-by: Lenz Weber-Tronic <mail@lenzw.de>
Still leaves the problem that there is a bunch of bundlers out there that can never be configured to replace What about Config for webpack would look like config.plugins.push(
new webpack.DefinePlugin({
"globalThis.process": JSON.stringify({ process: { NODE_ENV: "production" } }),
})
); For reference, I compiled a list of configurations for different bundlers over in this issue: apollographql/apollo-client#10915 |
I don't know about bunch of, but webpack from Next.js replace fine
I guess you wanted to say |
NextJs is moving away from Webpack, and Vite uses
But they can be configured to replace it, while we are talking about the alternative of locking a big part of the ecosystem out of these optimizations completely. One extra line of bundler config won't kill you, if that means that all |
@phryneas replaced to |
globalThis
with process
process.env
with globalThis
@phryneas you said
but I saw your reply #3918 (comment) where vite users can replace |
@B2o5T vite uses different bundlers for different environments, also |
Following the track of this error I got it for Nextjs version 13.5.6, using Graphql version 16.8.0 and I was able to fix it using the solution proposed by @phryneas in the comment: #3927 (comment) const nextConfig = {
webpack(config, { webpack }) {
config.plugins.push(
new webpack.DefinePlugin({
"globalThis.process": JSON.stringify({
env: {
NODE_ENV: "production",
},
}),
})
);
return config;
},
};
module.exports = nextConfig; |
fixes #3918 #3925
globalThis.process
andglobalThis.window
is a syntax that doesn't eliminated by webpackInput
Output (by webpack in Next.js)
Conclusion
The above code is a perfect case for running in a browser or to eliminate the opossite case (development dead code) by webpack for example.