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

Update computedFn warning to respect mobx global computedRequiresReaction #318

Merged
merged 2 commits into from
Jul 8, 2023

Conversation

wemyss
Copy link
Contributor

@wemyss wemyss commented Apr 11, 2023

Purpose

Addresses this issue: #268

This will allow consumers to test code wrapped with computedFn easily by using

mobx.configure({ computedRequiresReaction: false })

Currently in order to test a computed funciton you either:

  1. Ignore the warnings, e.g. --silent in jest
  2. Wrap the tested function in an autorun in each test or during setup before each test.

Notes

There will be a behavioural change in when a computedFn warning is triggered as the default for computedRequiresReaction is false. This will make it consistent in behaviour with the base computed, but if it is desirable to maintain the existing behavior we could add another option... I'm not sure where that would live, as mobx-utils has no global state and it seems odd to put computedFnRequiresReaction within the mobx global state - thoughts?

@ezze
Copy link

ezze commented Jul 6, 2023

@mweststrate @urugator Can this be merged, guys?

@NaridaL NaridaL merged commit 9aeb99b into mobxjs:master Jul 8, 2023
@urugator
Copy link
Contributor

urugator commented Jul 8, 2023

Sorry for the late response, but it should probably also respect requiresReaction flag from the options, taking precendece over global config, like here:
https://github.com/mobxjs/mobx/blob/27efa3cc637e3195589874990c23d4de82c12072/packages/mobx/src/core/computedvalue.ts#L311-L319

@NaridaL
Copy link
Collaborator

NaridaL commented Jul 8, 2023

Included in v6.0.7

@NaridaL
Copy link
Collaborator

NaridaL commented Jul 8, 2023

@urugator ah sorry, I've already published it :/

@urugator
Copy link
Contributor

urugator commented Jul 8, 2023

No problem, it's good you keep things moving forward here.

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.

4 participants