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

add allowStateChangesInsideComputed to allow changes inside a computed value #1706

Merged
merged 11 commits into from Sep 25, 2018

Conversation

xaviergonz
Copy link
Contributor

in order to fix MST mobxjs/mobx-state-tree#967

@xaviergonz xaviergonz changed the title make allowStateChanges allow changes inside computed value [WIP] make allowStateChanges allow changes inside computed value Aug 30, 2018
@xaviergonz xaviergonz changed the title [WIP] make allowStateChanges allow changes inside computed value [WIP] add allowStateChangesInsideComputed to allow changes inside a computed value Aug 30, 2018
@xaviergonz xaviergonz changed the title [WIP] add allowStateChangesInsideComputed to allow changes inside a computed value add allowStateChangesInsideComputed to allow changes inside a computed value Aug 30, 2018
@coveralls
Copy link

coveralls commented Aug 30, 2018

Coverage Status

Coverage increased (+0.02%) to 93.957% when pulling 1c55f6e on allow-state-changes-inside-computed into c86ba97 on master.

@xaviergonz
Copy link
Contributor Author

xaviergonz commented Aug 31, 2018

I changed the implementation so it only allows it for the current computed level, this is

computed -> changes state inside allowStateChangeInsideAction - ok
computed -> changes state inside allowStateChangeInsideAction -> computed -> changes state without allowStateChangeInsideAction - error

That way it is a bit better contained.

@mweststrate
Copy link
Member

@xaviergonz I just pushed an alternative solution, not sure whether I do like it better (I do like it better from api perspective, but not sure I like it better from the constraint that is being given up)

@xaviergonz
Copy link
Contributor Author

@mweststrate Thanks for taking a look!
TBH I'd rather not give up on the checks (that are useful for all users) altogether just for this feature (that's an internal one most probably only to be used by MST) TBH

src/core/action.ts Outdated Show resolved Hide resolved
@xaviergonz
Copy link
Contributor Author

@mweststrate should it go back to the original solution (the one that doesn't give up the constraint) in order to fix mobx-state-tree afterCreate?

@mweststrate
Copy link
Member

mweststrate commented Sep 24, 2018 via email

@xaviergonz
Copy link
Contributor Author

Well, it is only to fix that very particular use case :) Don't think people will (or even should) use it too much.
Reverted

@xaviergonz
Copy link
Contributor Author

Improved the unit test to show that:
computed -> afterCreate (change) will be ok
computed -> afterCreate (change) -> computed (other change) will throw

@mweststrate mweststrate merged commit fa09fa6 into master Sep 25, 2018
@xaviergonz
Copy link
Contributor Author

xaviergonz commented Sep 25, 2018

@mweststrate I was wondering, does this need to be retrofit to mobx 4 master as well?

@mweststrate
Copy link
Member

@xaviergonz yes, but no worries, already on it :)

@xaviergonz
Copy link
Contributor Author

ah thanks!

mweststrate pushed a commit that referenced this pull request Sep 25, 2018
…side a computed value (#1706)

* make allowStateChanges allow changes inside computed value

in order to fix MST mobxjs/mobx-state-tree#967

* added allowStateChangeInsideComputed

* added unit test

* small unit test error

* safer implementation

* Alternative solution

* Revert "Alternative solution"

This reverts commit e15bc78.

* improve unit test

* Changed to more predictable restoration
@mattruby
Copy link
Member

Thanks for keeping on this one!

@mweststrate
Copy link
Member

mweststrate commented Sep 25, 2018 via email

@danielkcz danielkcz deleted the allow-state-changes-inside-computed branch August 19, 2019 10:49
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.

None yet

4 participants