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

1.0.0: Use Map internally for MapSchema #62

Closed
endel opened this issue Apr 4, 2020 · 3 comments
Closed

1.0.0: Use Map internally for MapSchema #62

endel opened this issue Apr 4, 2020 · 3 comments
Assignees
Labels
Milestone

Comments

@endel
Copy link
Member

endel commented Apr 4, 2020

The MapSchema currently acts like a plain object in JavaScript, which lacks lots of useful methods that a Map would provide.

For version 1.0.0, the MapSchema should be re-structured to use a Map, and implement all methods of the Map structure: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map

@endel endel added this to the 1.0.0 milestone Apr 4, 2020
@endel endel added the feature label Apr 4, 2020
@endel
Copy link
Member Author

endel commented Apr 4, 2020

It would be a big plus to use tslint custom transform to automatically "upgrade" existing projects, to convert old MapSchema calls to newer ones, such as:

// old
const player = players["one"];
// new
const player = players.get("one");

// old
players["one"] = player;
// new
players.set("one", player);

@damian-pastorini
Copy link

Out of curiosity, use Map (or Set, also saw the other issue to include it), wouldn't affect performance over use Array?
And if that's the case, would this change be mandatory or could it be optional? Maybe you could keep all the different classes available for users choose which one suites us better?

@endel
Copy link
Member Author

endel commented May 30, 2020

Hi @damian-pastorini, regarding performance only benchmarks can tell, but it should be very little difference by using one or another (Map, Set, or Array).

I'll try to keep both syntaxes for maps so people upgrading won't feel much difference. When #75 lands, it should be possible to use either map.get("key") or map["key"]. But only map.get("key") will return the right type in TypeScript (#3)

endel added a commit that referenced this issue Jun 6, 2020
@endel endel self-assigned this Jun 11, 2020
endel added a commit that referenced this issue Jun 11, 2020
endel added a commit that referenced this issue Jun 13, 2020
@endel endel closed this as completed Oct 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants