Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

phryneas
Copy link

This could in the long term enhance early detection if users bundle multiple versions of the graphql package, and (in a future version of graphql) retire the current check that needs inside of instanceOf and thus needs to be restricted to development node, since it slows down the function.

@netlify
Copy link

netlify bot commented Jun 21, 2023

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit 422f59a
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/6492f8e5d6d45a00075295c4
😎 Deploy Preview https://deploy-preview-3915--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 21, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: phryneas / name: Lenz Weber-Tronic (422f59a)

@yaacovCR
Copy link
Contributor

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.

@phryneas
Copy link
Author

phryneas commented Sep 26, 2024

@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 /** PURE */ annotation.

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 graphql versions that take part use it. If we'd ship this in v17, but not v15 and v16, it would only warn on different v17 versions, but wouldn't trigger with v16 and v17 in parallel.

So: not sure if it really makes sense - on the other hand, if we never start shipping it, it will never start making sense.

@yaacovCR yaacovCR mentioned this pull request Sep 27, 2024
17 tasks
@yaacovCR
Copy link
Contributor

yaacovCR commented Sep 27, 2024

Does it matter that we have sideEffects set to false?

Does it depend on the bundler?

evanw/esbuild#1241 (comment)

@phryneas
Copy link
Author

phryneas commented Sep 27, 2024

sideEffects should stay false either way - while this is a side effect, it's not a very observable one or one that has an impact on anything than this package.

With sideEffects true, no tree-shaking will happen at all.

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 console.log as pure, but most don't) that cannot be determined to be 100% pure.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants