-
Notifications
You must be signed in to change notification settings - Fork 19
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
fix: rename caches to commonCaches #628
base: main
Are you sure you want to change the base?
Conversation
Renames the `caches` variable because it interferes with the global browser API. Fixes benjamn#492
@@ -131,7 +131,7 @@ export interface OptionsWithCacheInstance< | |||
cache: CommonCache<NoInfer<TCacheKey>, Entry<NoInfer<TArgs>, NoInfer<TResult>>>; | |||
}; | |||
|
|||
const caches = new Set<CommonCache<any, AnyEntry>>(); | |||
const commonCaches = new Set<CommonCache<any, AnyEntry>>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__caches
might be another alternative since it just seems like a local variable and it seems to be unintended to be accessed in the global scope.
Can you say more about why this causes problems? In my understanding, If the |
I wrote the #492 issue, but realize now I probably made a mistake when migrating from an IIFE bundle loaded as Thanks for pointing in the right direction! |
I ran into a similar issue trying to bundle apollo client with esbuild 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 |
Renames the
caches
variable because it interferes with the global browser API.https://developer.mozilla.org/en-US/docs/Web/API/caches
Fixes #492