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

Use WeakMap if available to avoid O(n) lookup #62

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

Conversation

sheerun
Copy link

@sheerun sheerun commented May 2, 2016

Supersedes #57

@gaearon
Copy link
Owner

gaearon commented May 2, 2016

Do you want to extract this to an npm module? Inspired by your previous PR I implemented something similar here because I need it in that project too. Ideally I’d prefer this be an external module I could depend on in both projects.

@sheerun
Copy link
Author

sheerun commented May 2, 2016

Isn't https://github.com/medikoo/es6-weak-map exactly what you're looking for?

@sheerun
Copy link
Author

sheerun commented May 2, 2016

var WeakMap;

if (useWeakMap) {
  WeakMap = require('es6-weak-map');
} else {
  WeakMap = require('es6-weak-map/polyfill');
}

@gaearon
Copy link
Owner

gaearon commented May 2, 2016

I don’t really care that much about real WeakMap semantics—it’s fine to leak memory in older browsers in development mode during hot reloading. The problem with polyfills is they sometimes have tricky edge cases or may cause weird problems in some environments. Ideally I want to:

  • Have full control over this code and understand it well.
  • Don’t let it bring in 10 other ES6 polyfills.
  • Use WeakMap when possible but otherwise I’m fine leaking memory.
  • Not define any fields on the components. This is how any true WeakMap polyfill works, but in my case I’d rather avoid it to avoid issues like this. Proxies are already invasive, and putting fields on components forces us to think about weird edge cases, which I’d rather avoid.

@sheerun
Copy link
Author

sheerun commented May 2, 2016

Do you wish to restrict input values to only objects? (just like WeakMap)

@gaearon
Copy link
Owner

gaearon commented May 2, 2016

Yeah, so that WeakMap case works.

@sheerun
Copy link
Author

sheerun commented May 2, 2016

I think https://github.com/WebReflection/es6-collections implementation should suit you except:

  1. It overrides global WeakMap
  2. It also implements few other collections

I could fix both and extract code to the new module. OK?

@gaearon
Copy link
Owner

gaearon commented May 2, 2016

I’d really prefer my implementation in this case. The surface area I need is very small, and putting components into “slots” by their displayName/name reduces the lookup time for the common case. I think we can benefit from having a very specific use case (map by component) here.

@sheerun
Copy link
Author

sheerun commented May 2, 2016

OK, I think I have full picture. I'll try to come up with something soon.

@wkwiatek wkwiatek changed the base branch from next to master March 6, 2017 09:04
@wkwiatek
Copy link
Collaborator

wkwiatek commented Mar 6, 2017

Have you come up with something new? Or should we just close this?

@sheerun sheerun force-pushed the feature/next-weakmap branch from 5f79811 to 6b0da8c Compare March 7, 2017 01:30
@sheerun
Copy link
Author

sheerun commented Mar 7, 2017

@wkwiatek Thanks for reminding.. I've created https://github.com/sheerun/component-map and update this PR to use it. ComponentMap includes lots of tests that also verify proper garbage collection. Hope you like it.

@sheerun
Copy link
Author

sheerun commented Mar 7, 2017

@gaearon You might want to consider using component-map it in react-hot-loader as well

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.

3 participants