Skip to content

(exchange-persisted-fetch) - Make indirect eval call to optimize for minifiers#1744

Merged
kitten merged 2 commits intourql-graphql:mainfrom
nderscore:indirect-eval
Jul 1, 2021
Merged

(exchange-persisted-fetch) - Make indirect eval call to optimize for minifiers#1744
kitten merged 2 commits intourql-graphql:mainfrom
nderscore:indirect-eval

Conversation

@nderscore
Copy link
Copy Markdown
Contributor

@nderscore nderscore commented Jun 26, 2021

Summary

The @urql/exchange-persisted-fetch package is not currently being minified to the fullest extent and even throws warnings in some minifiers/bundlers, such as esbuild, because it contains a direct call to eval().

Every minifier (including terser, which is being used to generate minified bundles of urql libraries) avoids mangling names when there is a direct eval call in scope. Because a direct call to eval gives the code being evaluated access to the scope from which it was called, there's no way for the minifier to know what side-effects that evaluted code has, so it avoids making optimizations.

By changing it to an indirect eval, we get smaller outputs, even though more boilerplate was added to the code:

  • urql-exchange-persisted-fetch.min.js shrinks from 3299b to 2802b 2792b
  • urql-exchange-persisted-fetch.min.mjs shrinks from 3281b to 2828b 2818b

This PR also resolves #875 which I believe was erroneously closed.

Set of changes

  • Replaced direct eval() call in exchanges/persisted-fetch/src/sha256.ts with an indirect call, guarded against errors with a try/catch.

Side-note

This is probably not the ideal fix. I question whether an eval here is even appropriate as a means of avoiding including crypto in a client-side bundle.

It would probably be better to generate separate builds for node and browser environments, where a direct call to require('crypto') gets tree-shaken out of the browser build.

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Jun 26, 2021

🦋 Changeset detected

Latest commit: b13b363

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@urql/exchange-persisted-fetch Patch

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

Comment thread exchanges/persisted-fetch/src/sha256.ts Outdated
@kitten
Copy link
Copy Markdown
Member

kitten commented Jun 27, 2021

The choice of not providing separate bundles is explicitly based on the currently fractioned ecosystem around package.json:exports. While we could provide separate bundles, it's questionable whether all environments currently would handle them correctly, and this often introduces an additional requirement on users to know what could go wrong, when things do go wrong.

That out of the way, I'll do a couple of spot checks next week to see whether this is working as intended ✌️

@kitten kitten merged commit 00ad642 into urql-graphql:main Jul 1, 2021
@urql-ci urql-ci mentioned this pull request Jul 1, 2021
@kitten
Copy link
Copy Markdown
Member

kitten commented Jul 1, 2021

@nderscore Can't find any problems with this, so let's ship it 💪 Thanks for taking a look at this!

@kitten
Copy link
Copy Markdown
Member

kitten commented Jul 6, 2021

shipped in @urql/exchange-persisted-fetch@1.3.2

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.

Avoid eval in persisted-fetch exchange

2 participants