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

WIP: Make observable map structurally match an ES6 Map #1361

Closed

Conversation

marcfallows
Copy link

@marcfallows marcfallows commented Mar 1, 2018

Observable map now has the typings to structurally match Map. Also implements toStringTag properly as if it were a Map. This should resolve #1204


Thanks for taking the effort to create a PR!

If you are creating an extensive PR, you might want to open an issue with your idea first, so that you don't put a lot of effort in an PR that wouldn't be accepted. Please prepend pull requests with WIP: if they are not yet finished
PR checklist:

  • Added unit tests
  • Updated changelog
  • Updated docs (either in the description of this PR as markdown, or as separate PR on the gh-pages branch. Please refer to this PR). For new functionality, at least API.md should be updated
  • Added typescript typings
  • Verified that there is no significant performance drop (npm run perf)

Feel free to ask help with any of these boxes!

The above process doesn't apply to doc updates etc.

@@ -13,7 +13,7 @@
"noImplicitAny": false,
"moduleResolution": "node",
"lib": [
"es5",
"es2015",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to change this so I could use Symbol.toStringMap and Symbol.iterator in the typings.

@@ -40,9 +41,19 @@ export interface IMap<K, V> {
get(key: K): V | undefined
has(key: K): boolean
set(key: K, value?: V): this
// prettier-ignore
[Symbol.toStringTag]: "Map"; // necessary semicolon
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prettier gets rid of this semi, but it is needed. Not familiar enough with prettier to know if there's a better way.

(Or maybe there's a better way to let TS know it's the end of the statement, but I don't know it).

@@ -22,7 +22,7 @@
"quick-build": "tsc --pretty",
"small-build": "node scripts/build.js",
"lint": "tslint -c tslint.json src/*.ts src/types/*.ts src/api/*.ts src/core/*.ts src/utils/*.ts",
"size": "size-limit 20KB lib/mobx.js",
"size": "size-limit 21KB lib/mobx.js",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently I've bumped the size over the limit by 10 bytes. I don't know if this is the change you would want - but this should get things to pass

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 63.339% when pulling 23cd761 on marcfallows:marc-observablemap-types into b246c19 on mobxjs:master.

@mweststrate
Copy link
Member

Thanks! Would it ok for you to target the mobx4 branch in this PR? That avoids a potential breaking change where people have to change their TS setup to be able to type Symbol.

@mweststrate mweststrate mentioned this pull request Mar 3, 2018
38 tasks
@marcfallows marcfallows changed the base branch from master to mobx4 March 4, 2018 23:49
@marcfallows
Copy link
Author

Ah, I hadn't realised there was a mobx4 branch with some of the TS configuration already changed. I'll have to resolve conflicts before this one is ready to go.

@marcfallows marcfallows changed the title Make observable map structurally match an ES6 Map WIP: Make observable map structurally match an ES6 Map Mar 4, 2018
@mweststrate
Copy link
Member

mweststrate commented Mar 5, 2018

@marcfallows don't bother, Ill merge your changes, that is propbably easier

@mweststrate
Copy link
Member

Merged, thanks!

@mweststrate mweststrate closed this Mar 5, 2018
mweststrate added a commit that referenced this pull request Mar 5, 2018
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