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

id-map should be modernized and existing logic moved to legacy #10703

Open
mitar opened this issue Sep 23, 2019 · 10 comments
Open

id-map should be modernized and existing logic moved to legacy #10703

mitar opened this issue Sep 23, 2019 · 10 comments

Comments

@mitar
Copy link
Contributor

mitar commented Sep 23, 2019

Now that we have modern/legacy browser separation, we could change id-map to simply be a Map with an existing id-map API on modern browsers, while keeping existing code to be used on legacy browser.

Alternatively, we could even replace id-map as a whole with simply a Map now, leaving to polyfills to implement Map to work over every key value. Interestingly though, these days id-map (since 79ae184), id-map is already using internally Map, but still does key stringification. That is not necessary anymore.

I am suspecting this would improve performance of Minimongo on the client as id-map is used there a lot. Moreover, id-map is used also in oplog driver on the server.

@stale
Copy link

stale bot commented Dec 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale-bot label Dec 10, 2019
@mitar
Copy link
Contributor Author

mitar commented Dec 10, 2019

Still relevant.

@stale stale bot removed the stale-bot label Dec 10, 2019
@mitar mitar mentioned this issue Dec 10, 2019
@filipenevola filipenevola added the confirmed We want to fix or implement it label Dec 11, 2019
@sebakerckhof
Copy link
Contributor

sebakerckhof commented Jan 7, 2020

@mitar The stringification is necessary in case you use mongo object ids.

@mitar
Copy link
Contributor Author

mitar commented Jan 7, 2020

Good point.

@sebakerckhof
Copy link
Contributor

Do you want to keep this open? Maybe we could check the type before calling stringify, but I doubt it will bring any meaningful improvement.

@mitar
Copy link
Contributor Author

mitar commented Jan 7, 2020

It think it would be useful to determine if this is performance benefit for string IDs. If it is large, then it could be useful to have two code paths based on ID (we know which ID is being used per collection).

@stale
Copy link

stale bot commented Oct 31, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale-bot label Oct 31, 2020
@mitar
Copy link
Contributor Author

mitar commented Oct 31, 2020

I think this has been done? No? We are using map internally now?

@stale stale bot removed the stale-bot label Oct 31, 2020
@StorytellerCZ StorytellerCZ added ready We've decided how to solve or implement it good first issue Good first issue or something that should is nice to do. Project:Minimongo Project:Mongo Driver Project:DDP labels May 7, 2021
@StorytellerCZ
Copy link
Collaborator

Just checked. As @mitar has stated id-map is using Map internally, but I haven't seen any move in DDP or MiniMongo to move to just a Map so the question is if we want to do more here or are we in a happy place at the moment.

@StorytellerCZ StorytellerCZ removed good first issue Good first issue or something that should is nice to do. ready We've decided how to solve or implement it labels May 7, 2021
@mitar
Copy link
Contributor Author

mitar commented May 11, 2021

Maybe open another issue for those two?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants