search

mobxjs / mobx-state-tree

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

Symbol as model key #1939

Open Yuu6883 opened 2 years ago

Yuu6883 commented 2 years ago

Feature request

Is your feature request related to a problem? Please describe. Trying to replace some large nested mobx observable objects with MST model, but unable to do so because MST model ignores Symbol keys when mobx itself supports observable Symbol keys.

Describe the solution you'd like Symbol as key in a model, by possibly replacing Object.keys in https://github.com/mobxjs/mobx-state-tree/blob/master/packages/mobx-state-tree/src/types/complex-types/model.ts with Reflect.ownKeys

Describe alternatives you've considered Replacing all occurrences of symbol with plain strings in my code, but need to make addition string maps because I'm also using Symbol to store a description string field in it.

Additional context

Are you willing to (attempt) a PR?

coolsoftwaretyler commented 1 year ago

Hey @Yuu6883 - thanks for filing this issue, and I'm sorry it took so long for us to get back to you.

I think this is an interesting idea, but I'm not sure when or how it'll fall on the priorities list. I'm going to label it as an enhancement, open for contribution. Since you said you weren't interested in a PR, I'd be happy to chat with anyone who comes across this and wants to give it a go.

singularvoice commented 1 year ago

Hey @coolsoftwaretyler I would be happy to do this issue, It would be my first contribution to mobx-state-tree. Can you please tell me some more detail about this issue

coolsoftwaretyler commented 1 year ago

Hi @a-hassanzadeh-h - thanks for your interest in contributing (and for your other PR!).

I think to get started, I would do a few things:

  1. Write some tests that use symbols as keys on a model. Verify that they fail as expected. You might write those in packages/mobx-state-tree/__tests__/core/model.test.ts
  2. Take a look at the code that the author of this issue linked, and see if making changes there gets the tests to pass.
  3. Consider any other implications of your change, check the test suite, add tests for edge cases that might come up.

Let me know if you have specific questions once you dig in. We can troubleshoot here as needed. FYI, I am a bit busy in the coming weeks. Apologies for delays on response.

singularvoice commented 1 year ago

Hey @coolsoftwaretyler, Sure. Okay, I am going to work on it, I will keep you updated if I have questions.