new __DEV__ handling #10915
Merged
new __DEV__ handling #10915
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 03ff6c4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
/release:pr |
|
Please add a changeset via |
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
/release:pr |
|
A new release has been made for this PR. You can install it with |
size-limit report 📦
|
as the `undefined` check only works in the built lib, not the sources
|
/release:pr |
|
A new release has been made for this PR. You can install it with |
|
/release:pr |
|
A new release has been made for this PR. You can install it with |
|
So the good news is that even removing that
Uploaded test files are here:
Unfortunately, the server will run into CORS problems, so for more testing you'll have to download them and run them from The question is if it also works in all the other envs... |
3 tasks
|
For now, I will remove the |
|
Here is a diff of all built files: differences in *.js files
|
jerelmiller
approved these changes
Jul 5, 2023
| @@ -4,6 +4,7 @@ on: | |||
| branches: | |||
| - main | |||
| - release-* | |||
| - pr/* | |||
There was a problem hiding this comment.
I'm assuming this was for testing this branch. Do you still need this?
There was a problem hiding this comment.
It will generally be useful for PRs against other PR branches.
This was referenced Aug 2, 2023
Merged
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
reverts #10521 (Simplify DEV polyfill to use imports instead of global scope)
more changes to come here, I just want to have a CI build already
Checklist:
On handling of
process.env.NODE_ENVor__DEV__I am trying to find a way here that doesn't crash, doesn't need us to globally polyfill
process.env.NODE_ENVor__DEV__,and still can be tree-shaken away in production builds.
Since we cannot assume that one of those values is always defined, we would want the "undefined" case to be handles as if
__DEV__wastrue.Problems
process.env.NODE_ENVor__DEV__are not defined in the browserThis is our baseline problem - a bundle with a naive check for either of those can be handled very gracefully by the bundler,
but will crash in-browser ESM imports.
Bundlers are stupid
Usually, bundlers rely on plain string replacement to replace
process.env.NODE_ENVor__DEV__with a primitive value.That means that in the case of
process.env.NODE_ENV, the bundler will replaceprocess.env.NODE_ENVwith"production"or
"development"- but it will not fill in forprocessorprocess.env.So the bundler could not statically analyze "resilient" code like
typeof process !== "undefined" && process.env.NODE_ENV === "development".In the case of
__DEV__, things become a bit easier since we don't have to check forprocessorprocess.env- simply replacing__DEV__inthe statement
typeof __DEV__ == "undefined" || __DEV__will for example result intypeof false == "undefined" || false.Now, it depends on what the bundler does with
typeof false == "undefined".Unfortunately, most bundlers will handle this as non-evaluated runtime statement. In that case, it could not tree-shake.
We could now try to let the bundler replace
typeof __DEV__with"undefined"- but not all bundlers allow for string replacement on that level. (esbuild has no option for this)Defeating it just with logic
Finding a logical statement that, naively replaced by the bundler, can eliminate the
typeofwould work if we would only go for__DEV__ === true, but not for__DEV__ === true || __DEV__ === undefined- in the latter case, there will always be some "runtime-type" statement left that the bundler cannot eliminate.Getting rid of the
typeofThis is the strategy I'm settling on now: If we can get rid of the
typeof, we get something that can always be replaced by bundlers.My current approach for that is to use
globalThis.__DEV__- this will not by default be replaced by most bundlers, but at least they can be configured to be replaced.The logical statement I've ended up is
globalThis.__DEV__ !== false.In the wild
{ "define": { "globalThis.__DEV__": "false" } }{ "global_defs": { "@globalThis.__DEV__": "false" } }.swcrc:{ "jsc": { "transform": { "optimizer": { "globals": { "vars": { "globalThis.__DEV__": "false" } } } } } }next.config.js:vite.config.tsusing craco
craco.config.jsSee this later comment for more detail