Skip to content

fix(4610): computed keepAlive_ 'true' by default#4612

Open
Tzahile wants to merge 10 commits intomobxjs:mainfrom
Tzahile:lehmann/fix(4610)-computed-default-keepAlive
Open

fix(4610): computed keepAlive_ 'true' by default#4612
Tzahile wants to merge 10 commits intomobxjs:mainfrom
Tzahile:lehmann/fix(4610)-computed-default-keepAlive

Conversation

@Tzahile
Copy link
Copy Markdown

@Tzahile Tzahile commented Feb 18, 2026

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)

This is a PR to fix #4610
keepAlive_ default value is changed to true.
As mentioned in the issue description:

With #1799 I don't see any downsides to having all computed values be keepAlive

This PR also

  • changes the warning message and its triggers (trigger now if keepAlive_ is false)
  • updates tests for v4 and v5
  • updates docs with new behaviour and examples

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Feb 18, 2026

🦋 Changeset detected

Latest commit: 38c8923

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
mobx Minor

Not sure what this means? Click here to learn what changesets are.

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

@Tzahile
Copy link
Copy Markdown
Author

Tzahile commented Feb 26, 2026

Modifications

  • Added a global flag

  • keepAlive is set by this order:

    • explicit set
    • global flag
    • defaults to true

    If a computed has no explicit keepAlive ->
    it will then check for global flag value. if there's no global flag ->
    then it will default to true.

@Tzahile Tzahile marked this pull request as draft February 26, 2026 13:29
fixed global flag when set by "false"/"true"
revert docs and added proper explanation about future plans
@Tzahile Tzahile marked this pull request as ready for review February 26, 2026 14:43
@mweststrate
Copy link
Copy Markdown
Member

Thanks for taking the effort of putting up this PR! However, as discussed in the issue (and happy to elaborate more), the current behavior is very deliberately by design. Apologies for my late follow up on the issue, that might have saved the trouble of setting up the PR. But thanks nonetheless for taking a stab at it!

@mweststrate
Copy link
Copy Markdown
Member

Thanks for taking the effort of putting up this PR! However, as discussed in the issue (and happy to elaborate more), the current behavior is very deliberately by design. Apologies for my late follow up on the issue, that might have saved the trouble of setting up the PR. But thanks nonetheless for taking a stab at it!

In theory the PR could still be useful except for changing the default part. What makes me pause here is that it would alter the behavior of any dependency that would also rely on MobX, and might break it as they would assume the current behavior without any override.

@Tzahile
Copy link
Copy Markdown
Author

Tzahile commented Mar 9, 2026

Hi @mweststrate
No problem at all, I'm happy to have the opportunity to learn and contribute to a tool I use a lot.

I can make the changes so that my PR is still valuable, could you then please specify what I should do?
Or reverting the default value is the only thing you think should be changed?

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.

Computed values should keepAlive by default

2 participants