-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Simplify __DEV__
polyfill to use imports instead of global scope
#10521
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
fc9c86f
to
a044507
Compare
I expected this For comparison, the bundlesize is 32.41KB right now on |
Okay, the/one reason the bundle size regression isn't so bad is that we use Rollup to generate our all-in-one function getDEV() {
return "__DEV__" in global$1
? Boolean(global$1.__DEV__)
: maybe(function () { return process.env.NODE_ENV; }) !== "production";
}
var DEV = getDEV();
var __DEV__ = DEV;
It appears Webpack now applies It's encouraging that ESM tooling seems to be getting this right by default now, whereas previously the ESM-to-CJS compilation step threw a 🔧 into the works. I'm still a little worried about regressing bundle sizes for existing apps that compile all the way to CommonJS, but perhaps that's (more) tolerable since compiling to ESM is getting easier and is more often the default behavior. |
See #10521 for discussion of concerns about this change.
export * from './common/responseIterator'; | ||
export * from './common/incrementalResult'; |
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.
Explaining the changes from 4207078:
When adding new exports to @apollo/client/utilities
, the exported stuff needs to be accessible from the top-level @apollo/client/utilities
package if it's going to be used elsewhere within @apollo/client
(outside /utilities
). This problem is flagged during npm run build
by warnings like
Risky cross-entry-point nested import of ../version in core/ApolloClient.js
...
Risky cross-entry-point nested import of ../../react/cache in testing/react/MockedProvider.js
Risky cross-entry-point nested import of ../../react/cache/index.js in testing/react/MockedProvider.js
These are just warnings because they are not always a problem: cross-entry-point code may be unintentionally duplicated across CJS bundles, which can be a problem if the code defines a class
that's only supposed to have one definition (for example), but in other cases the duplication is convenient and harmless, such as when the CJS bundle is used only for testing, or the duplicated code is safe to duplicate (like the import of ../version
from within core/ApolloClient.js
).
re:
Much if this from the new |
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.
🚢 This looks good to me! Really appreciate you diving into this right away!
Oh ya, and don't forget to add a changeset ( |
/release:pr |
A new release has been made for this PR. You can install it with |
Nice, thanks to that release, I'm confident this version of the library still works without bundling, as you can see by running this code in your browser console: await import("https://cdn.jsdelivr.net/npm/@apollo/client@0.0.0-pr-10521-20230206180228/+esm") |
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 for the background! Excited to have people try out the PR/alpha build with these changes 😄
__DEV__
polyfill to use imports instead of global scope__DEV__
polyfill to use imports instead of global scope
This PR substantially undoes much of the complexity introduced by #8347, at the cost of regressing bundle sizes by approximately 3.75KB for any application where the
__DEV__
imports get transpiled to something else before the minifier has a chance to prune unreachable code, such asglobals.__DEV__
following a CJS import likevar globals = require("../../utilities/globals")
.If compilers/transpilers/bundlers would just leave the imported
__DEV__
identifiers alone, this would all work very nicely, but the ESM transpilation behavior of third-party toolchains is not something we can control.Relevant past work:
__DEV__
internally instead ofprocess.env.NODE_ENV
. #8347__DEV__
from mis-minification / aggressive tree-shaking #8393__DEV__
is polyfilled in every entry point that uses it #8689@apollo/client/utilities/globals
wherever__DEV__
is used #8720__DEV__
declaration conflict with React Native (when"skipLibCheck": false
intsconfig.json
) #9386