search

benjamn / optimism

Composable reactive caching with efficient invalidation.
MIT License
114 stars 19 forks source link

fix: rename caches to commonCaches #628

Open lekoaf opened 11 months ago

lekoaf commented 11 months ago

Renames the caches variable because it interferes with the global browser API.

https://developer.mozilla.org/en-US/docs/Web/API/caches

Fixes #492

benjamn commented 11 months ago

Can you say more about why this causes problems? In my understanding, index.ts is not global scope, but (gets compiled to) a JS module, so it has its own top-level module scope, distinct from global scope.

If the optimism package code is getting compiled to something other than ESM or CommonJS modules, and the top-level scopes of the modules are all becoming global, that's very much not the intended usage.

steabert commented 11 months ago

I wrote the #492 issue, but realize now I probably made a mistake when migrating from an IIFE bundle loaded as text/javascript to ESM with code splitting loaded as module. The only way I can reproduce the issue is if I create an ESM bundle and then load it as text/javascript in the browser. Since it's bundled, there are no ESM features that could trip up the browser, so I didn't notice that.

Thanks for pointing in the right direction!

impaler commented 3 months ago

I ran into a similar issue trying to bundle apollo client with esbuild bundle:true the issue was not having a <script type="module" for the module.

I guess this PR is kind of a subjective request, library authors have no technical reason why they can't own their own namespace. It just seems like a common convention to avoid browser window apis for perhaps "readability". eg a routing library that supports the browser, would probably never have a navigator const in their code, even if they can safely?