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

Avoid crash if WeakSet is not available #167

Merged
merged 3 commits into from
Mar 8, 2019
Merged

Conversation

jgoz
Copy link
Contributor

@jgoz jgoz commented Mar 8, 2019

WeakSet is not available in IE11, so using react-window in development mode causes a runtime error.

This simply falls back to Set if WeakSet is not available. This could have memory implications since Set will hold onto the references indefinitely, but that only affects the one browser and only in development mode. It's also a much smaller impact on bundle size vs., say, a polyfill.

@bvaughn
Copy link
Owner

bvaughn commented Mar 8, 2019

I know this is a small change in terms of LOC, but I'm having a strong initial negative reaction to it. I think because I don't like the idea of leaking memory (even in DEV mode) when there's a reasonable workaround, e.g.:

if (process.env.NODE_ENV !== 'production') {
  if (typeof window.WeakMap === 'undefined') {
    window.WeakMap = require('es6-weak-map/polyfill');
  }
}

@jgoz
Copy link
Contributor Author

jgoz commented Mar 8, 2019

Yeah, I definitely see your point. What about simply not initializing the sets if WeakSet isn't available? IE11 wouldn't get the warnings in dev mode, but at least it wouldn't crash.

@jgoz jgoz changed the title Use Set if WeakSet is not available Avoid crash if WeakSet is not available Mar 8, 2019
@bvaughn bvaughn force-pushed the master branch 2 times, most recently from 3153343 to c14bce6 Compare March 8, 2019 04:00
@bvaughn
Copy link
Owner

bvaughn commented Mar 8, 2019

I like this approach much better. Thanks

@bvaughn bvaughn merged commit a669fc9 into bvaughn:master Mar 8, 2019
@jgoz jgoz deleted the weakset-ie11 branch March 8, 2019 15:59
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