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

Observable decorator on computed symbol keys #2175

Merged
merged 6 commits into from
Nov 7, 2019
Merged

Observable decorator on computed symbol keys #2175

merged 6 commits into from
Nov 7, 2019

Conversation

StephenHaney
Copy link
Contributor

@StephenHaney StephenHaney commented Oct 23, 2019

Resolves #2159

  1. I added a new test in typescript-tests that tries to use computed property keys... see test #2159 - computed property keys. Please validate this test is a supported use case (I think it is). It was failing before this change (see original issue for details). By the way, I verified the test passes if the key isn't a computed symbol to make sure the test itself is true.

  2. Changed decorators.ts and observable.ts to apply their logic to symbol keyed properties in addition to string keyed properties.

PR checklist:

  • Added unit tests
  • Updated changelog
  • Updated /docs. For new functionality, at least API.md should be updated
    I don't think this fix needs any doc updates.
  • Added typescript typings
    As all changes were made in TS files, no need.
  • Verified that there is no significant performance drop (npm run perf)
    Note: I didn't see npm run perf but I tried out npm run test:performance
    Current develop branch: 66.22 real 67.58 user
    This PR: 62.46 real 63.76 user

@StephenHaney
Copy link
Contributor Author

StephenHaney commented Oct 25, 2019

Tests are now passing, including the new one for computed symbol property keys. If we think this is a good idea, I'll finish up this PR with changelog updates, etc.

@urugator
Copy link
Collaborator

urugator commented Oct 25, 2019

Btw there seems to be quite a lot of functions/objects expecting string as a prop key. I suppose changing it to string | symbol could help preventing these bugs (as a hint for programmer)?

@StephenHaney StephenHaney changed the title WIP: Decorator checks for typeof symbol now too, plus tests... tests still crashing Observable works for computed symbol keys Oct 25, 2019
@StephenHaney
Copy link
Contributor Author

StephenHaney commented Oct 25, 2019

I suppose changing it to string | symbol could help preventing these bugs

I started going down this path but due to the TS limitation around using symbols as index keys, we'd end up having to cast it back to string everywhere to avoid TS compile errors. After TS patches that limitation, I think it'd be a good idea to use the PropertyKey type everywhere.

@StephenHaney StephenHaney changed the title Observable works for computed symbol keys Observable decorator for computed symbol keys Oct 25, 2019
@StephenHaney StephenHaney changed the title Observable decorator for computed symbol keys Observable decorator works for computed symbol keys Oct 25, 2019
@StephenHaney StephenHaney changed the title Observable decorator works for computed symbol keys Observable decorator on computed symbol keys Oct 25, 2019
@mweststrate mweststrate merged commit 4d56bf6 into mobxjs:master Nov 7, 2019
@mweststrate
Copy link
Member

Published as 5.15.0

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.

Decorators make properties readonly if their key is a symbol computed property in TS (phew)
3 participants