Skip to content

fix: throw when a model member is redeclared across actions, views or volatile state - #2304

Open
cpruijsen wants to merge 3 commits into
mobxjs:masterfrom
cpruijsen:fix/issue-2208-02c4470a
Open

cpruijsen wants to merge 3 commits into
mobxjs:masterfrom
cpruijsen:fix/issue-2208-02c4470a

Conversation

@cpruijsen

@cpruijsen cpruijsen commented Oct 12, 2026 •

Copy link
Copy Markdown

What does this PR do and why?

In development mode, or in production after setMemberRedeclarationChecking(true), MST now throws at create() when a model declares the same member name as two different kinds, for example an action and then a view called anAction. Before, the later declaration silently replaced the earlier one (#2208). The only existing check compared member names against properties.

The message follows the existing property-collision wording:

[mobx-state-tree] 'anAction' already exists as an action and cannot be declared as a view

The check lives in src/types/complex-types/model.ts and also applies through .extend(). Member names and kinds are recorded per instance in a module-level WeakMap while the initializers run, and the entry is deleted once the instance is finalized.

Judgement calls:

  • Only cross-kind collisions throw. A second .actions() or .views() with the same name still overrides the first, because existing tests rely on it (composable actions, two afterCreate hooks, override through compose) and the issue discussion asks to keep those patterns. If you want same-kind duplicates to throw too, I can change it.
  • types.compose keeps its override semantics. Each composed type starts with an empty name record, so types.compose(LoudPerson, QuietPerson) still lets the second type override the first, also across kinds. Duplicates inside one composed type still throw.
  • Known limitation: because of that reset, a member chained after a compose (types.compose(A, B).views(...)) can still override a member of A of a different kind without an error, as it does today. Fixing it needs per-operand bookkeeping and a decision on the semantics, so it is left out.
  • Property versus member collisions across composed types (thegedge's comment on the issue) are unchanged.

Fixes #2208

Steps to validate locally

The four new collision tests in __tests__/core/model.test.ts (action then view, view then action, action then volatile state, volatile state then view through .extend()) fail without the change to model.ts and pass with it. Two more tests confirm that same-kind redeclaration and cross-kind override through types.compose still work.

@coolsoftwaretyler

Copy link
Copy Markdown
Collaborator

Thanks, I'll take a look soon!

@coolsoftwaretyler

Copy link
Copy Markdown
Collaborator

Hey @cpruijsen - this is looking pretty good, although I notice a small slowdown in model creation, which we're already sensitive to in existing code.

Some of our other checks are already gated behind dev mode. Would you mind moving this check behind dev mode instead, along with an option to enable it in production if users want to opt in to slower, but more correct checking in prod?

Let me know if you need a hand with those suggestions. Thanks!

@cpruijsen

Copy link
Copy Markdown
Author

I moved the check behind dev mode, so production skips it by default. To turn it on in production, call setMemberRedeclarationChecking(true). It follows the setLivelinessChecking pattern, with a matching getter, and it is exported and documented next to that setter. Tests cover dev mode, the production default and the production opt-in. If you prefer an env var like ENABLE_TYPE_CHECK, I can change it.

This branch has not been deployed

No deployments
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.

Model Creation/Instantiation Allows Duplicate names (except for properties)

2 participants