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

Added useMap option to graph #22

Merged
merged 2 commits into from
Nov 10, 2019
Merged

Added useMap option to graph #22

merged 2 commits into from
Nov 10, 2019

Conversation

FelixMo42
Copy link
Contributor

@FelixMo42 FelixMo42 commented Nov 3, 2019

Allow people to use maps instead of objects because

  1. they are faster (755 ops/sec ±1.45% vs 6,462 ops/sec ±3.79%)
  2. they allow you to use objects as keys.

I would say make it the default, but not all browsers support it. Maybe make it check to see it it's supported and automatically enable it.

I made sure it passed all the tests.

PS, I love ngraph. I just started using it and it's awsome.

@FelixMo42
Copy link
Contributor Author

@anvaka hi, could you have a look at this. Sorry, this is my first time contributing to something like this, I'm not sure what proper protocol is.

@anvaka
Copy link
Owner

anvaka commented Nov 10, 2019

Hi @FelixMo42

Thank you for your contribution! I want to take a deeper look into your change and will come back within couple days.

@anvaka anvaka merged commit 915737c into anvaka:master Nov 10, 2019
@anvaka
Copy link
Owner

anvaka commented Nov 10, 2019

Love your change. Merged it in. Before publishing a new version, I'm going to update ngraph.graph so that it uses Map by default (no need to pass an option). Will probably need to think a bit more about for .. of operator as it seem to be not supported in IE.

@anvaka
Copy link
Owner

anvaka commented Nov 10, 2019

Okay, updated package to use the Map, and removed bunch of old code. Your performance test jumped from 6K ops per second to 12K.

I still haven't published this as a new version. Want to pull from my repository and play with it to see if it works for you?

@FelixMo42
Copy link
Contributor Author

I gave it a run and it looks good to me!

@anvaka
Copy link
Owner

anvaka commented Nov 10, 2019

Thank you! Published as v19.0.0

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.

None yet

2 participants