-
Notifications
You must be signed in to change notification settings - Fork 2k
suggestion: alternative check for multiple graphql packages #3915
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
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.