-
Notifications
You must be signed in to change notification settings - Fork 2k
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
suggestion: alternative check for multiple graphql packages #3915
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 settings. |
|
This is really interesting! Is there the possibility that this side effect might get pruned by tree shaking? Might we need to list it formally as a side effect? Note also that the current check emits a different instanceof function entirely so it also runs only once. |
@yaacovCR something like this shouldn't get tree-shaken, as it's not a declaration, but a method-call, which bundlers will assume as "with side-effects", unless you add a And even if it gets tree-shaken: nothing will break, it will just not do the check. That said, this approach has the drawback that it will only work if all So: not sure if it really makes sense - on the other hand, if we never start shipping it, it will never start making sense. |
Does it matter that we have sideEffects set to false? Does it depend on the bundler? |
With Beyond that, it is up to the bundler, but as far as I'm aware I haven't seen a bundler that has removed a top-level function call (that was not explicitly marked as pure) in a file that was imported for some other reasons. The examples you linked to are not dropped because they are function calls (some bundlers mark Tree-shaking usually either drops full files (if none of the exports are used), or export definitions that are pure, either by annotation or by heuristics. |
This could in the long term enhance early detection if users bundle multiple versions of the
graphql
package, and (in a future version ofgraphql
) retire the current check that needs inside ofinstanceOf
and thus needs to be restricted to development node, since it slows down the function.