-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[request] don't deprecate Mobx.transaction #1139
Comments
A bit unrelated, but I believe that an existence of transaction (or whatever the name should be), which doesn't call |
I concur. I even notice that it helps sometimes to first explain
transactions, and after that actions. @urugator good point about #940.
Op do 17 aug. 2017 om 13:55 schreef urugator <[email protected]>:
… A bit unreleated, but believe that an existence of transaction (or
whatever the name should be), which doesn't call allowStateChanges, could
help with an implementation of some observable structures, while respecting
the strict mode, see 940
<#940 (comment)>.
(in such case it's not a naming issue, but actually an implementation
issue)
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#1139 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABvGhOiUQtES1AwZNQp6Jv2MGqO_AcwLks5sZCo4gaJpZM4O6D_4>
.
|
Cool! Should we report this in a todo somewhere? |
That is what this issue is for ;-)
Op do 17 aug. 2017 om 17:36 schreef Daniel Neveux <[email protected]
…:
Cool! Should we report this in a todo somewhere?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1139 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABvGhKvR-FQXoOQKY3MjsuZhAatZpFQPks5sZF3HgaJpZM4O6D_4>
.
|
3.3.0 removed the deprecated message, 3.3.1 will remove the action semantics it has now. |
Fixed by 3.3.1 |
Idea: To not deprecate Mobx.transaction
Selfish complaint:
I regret to see transaction being deprecated. I know
transaction
is the same thing thatrunInAction
at the implementation level. But at API level, I feel inconfortable to be tight to the Mobx concept of an "Action". It does not make sense for the usage I want.I am implementing the SAM pattern where a mutation does not happen in the action but in the model.
Exactly like with Vuex for Vue.js, an action just prepares the data and sends it to the mutations. Action and mutation have two separated concerns here.
So until now I was using transactions which sounds better and just makes what I want: to retain the observers during the mutations.
More general complaint
What (I think) makes Mobx great is this non opinionated usage. Keeping access to (even if it is an alias) low level API which does something simple helps to do more things. At the cost to have a larger API surface.
It is not an implementation issue as the source shows that`runInAction does the same thing.
It is a naming issue (and naming thing is always important).
As Mobx is unopinionated:
it could be used to implement any pattern which comes with its own semantic. Actually you can. But at the cost of wrapping some function to rename it. Eg. in the future
const transaction = runInAction;
That could seem insignificant, but I am sure you have carefully named the API based on the concern of each function and you vision of Mobx. That means future implementation could potentially break this simple wrap.
The point is Action was created as a helper for functions to be wrapped with untracked, transaction and allowStateChanges.
That means Action has enhanced Mobx to a higher level concept. And that is great cause this means potentially more things done with less code. But I think lower concept like
transaction
should be part of the game.General benefits
Keeping liwer API could be a guard for Mobx to keep its original unopinionated vision, and to not shift to a high level API which make it less usable for implementing lower level pattern.
In short, do you think we could have a chance to not deprecate this function?
The text was updated successfully, but these errors were encountered: