search

mobxjs / mobx-state-tree

Full-featured reactive state management without the boilerplate
https://mobx-state-tree.js.org/
MIT License
6.9k stars 639 forks source link

Add number types to map model methods #2079

Closed singularvoice closed 9 months ago

singularvoice commented 10 months ago

What does this PR do and why?

Fix mobxjs/mobx-state-tree#1845

Steps to validate locally

singularvoice commented 10 months ago

@chakrihacker Hi Thanks for the review, I will consider your comments and push new commits. Unfortunately, I am a little busy in this week, but I will do it the next week.

coolsoftwaretyler commented 9 months ago

Hey @a-hassanzadeh-h - any update on your end for this?

singularvoice commented 9 months ago

Hey, @coolsoftwaretyler Last week I was swamped, but I worked on it the week before. My problem is I don't know which test I should write to consider this issue as done after tests get passed. Sorry for this very Newbie question, but I am very new to this thing, and I like to contribute I just need some help to start. Could you help me with tests?

coolsoftwaretyler commented 9 months ago

Hey @a-hassanzadeh-h - all good! I am happy to help you get started there.

So I think @chakrihacker is asking you to look at the test file in packages/mobx-state-tree/__tests__/core/map.test.ts. What you'll want to do locally is:

  1. yarn build to build your local version of the library
  2. yarn test to see what's passing/failing.

I think on line 449, there's an as any which Subramanya is referring to. The idea is that before your change, we would expect a typescript error with that change, and after your PR, we should see no error.

coolsoftwaretyler commented 9 months ago

Hey @chakrihacker - it looks like @a-hassanzadeh-h has pushed a new commit here. Do y'all want to see if you can keep pushing this forward? Happy to facilitate if you need any additional help.

chakrihacker commented 9 months ago

I will check today

chakrihacker commented 9 months ago

One small thing left, can you update the test case??

coolsoftwaretyler commented 9 months ago

Hey @chakrihacker - what do you think about updating the test case on their behalf? This PR has been open a while and I'd like to either merge it or close it before this weekend so I can do some work on the repo restructure.

chakrihacker commented 9 months ago

@coolsoftwaretyler I have updated, sorry forgot about this one

coolsoftwaretyler commented 9 months ago

All good! Thanks @a-hassanzadeh-h and @chakrihacker!

cmdcolin commented 8 months ago

this PR appears to make things change from just a string type to a string|number in a number of places, even for codebases that just use strings. would it be an alternative approach (or does it already exist?) to specify e.g. types.map<string>(ModelType) to say that the keytype is strings?