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

Symbol as model key #1939

Open
1 of 2 tasks
Yuu6883 opened this issue Jul 27, 2022 · 4 comments
Open
1 of 2 tasks

Symbol as model key #1939

Yuu6883 opened this issue Jul 27, 2022 · 4 comments
Labels
enhancement Possible enhancement help/PR welcome Help/Pull request from contributors to fix the issue is welcome level: intermediate

Comments

@Yuu6883
Copy link

Yuu6883 commented Jul 27, 2022

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?

  • Yes
  • No
@coolsoftwaretyler coolsoftwaretyler added enhancement Possible enhancement help/PR welcome Help/Pull request from contributors to fix the issue is welcome level: intermediate labels Jun 25, 2023
@coolsoftwaretyler
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Possible enhancement help/PR welcome Help/Pull request from contributors to fix the issue is welcome level: intermediate
Projects
None yet
Development

No branches or pull requests

3 participants