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

inheritance should work with computed #3950

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jzhan-canva
Copy link
Contributor

Attempt to fix #3949

Code change checklist

  • Added/updated unit tests
  • Updated /docs. For new functionality, at least API.md should be updated
  • Verified that there is no significant performance drop (yarn mobx test:performance)

Copy link

changeset-bot bot commented Oct 22, 2024

⚠️ No Changeset found

Latest commit: bee3e32

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@urugator
Copy link
Collaborator

Similar to #3902 it diverges from established rules:
https://mobx.js.org/subclassing.html

Field can't be re-annotated in subclass, except with override.

https://mobx.js.org/observable-state.html#limitations

Every field can be annotated only once (except for override). The field annotation or configuration can't change in subclass.

If we need different set of rules for modern docorators it should be at least documented.

Also we have subclassing tests for legacy decorators, they should be migrated/adapted:
https://github.com/mobxjs/mobx/blob/main/packages/mobx/__tests__/v5/base/make-observable.ts#L622-L1704

@jzhan-canva
Copy link
Contributor Author

jzhan-canva commented Oct 22, 2024

@urugator I personal lean toward a new set of rules for modern decorators, it's already very different from annotation in makeObservable
for example as you pointed out action can be re-annotated.
also in stage3-decorators test, we already have a inheritance overrides observable, although we probably miss putting extends A (the test still pass if I add extends A)
image
also @override is not supported in modern decorators

once we have computed to behave the same, they will be more predictable

@urugator
Copy link
Collaborator

Sure, but lets define what the rules are, what the expected behavior is, what the limitations are, how it should or shouldn't work with rest of the current api (makeObservable, ...), what can we do to prevent missuse, document it and test it. It should have been done with the introduction of the new decorators support, but here we are. The new decorators were introduced as almost 1-1 replacement for the old ones, turns out it's not.
The migration guide is incomplete, documentation on subcassing is not up to date, annotations and limitations are not correct, the undecorate tool probably needs to be updated, etc.
I don't want to prevent pushing things further, but we should not just continue to ignore this.

Regarding the fix itself I can't provide much insight, because:

  1. I was not invested in the impl of ES decorators support.
  2. No expectations other than the existing ones were set for ES decorators.

@mweststrate
Copy link
Member

I'm inclined to treat MobX 6 as transitional, and focus on what the sane behaviors would be in MobX 7, without conflating and needing to be consistent with either legacy decorators or makeObservable+decorators combo.

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.

[Bug] 2022-03 decorators doesn't support inheritance with @computed
3 participants