-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Patch] Remove silent option from mutations #457
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
Conversation
blake-newman
commented
Nov 14, 2016
- Now Dev tools supports filtering silent option is no longer required
- Updated unit and type tests
Maybe we should deprecate the option and provide warning for users at first? |
Yeah, this would technically be a breaking change. Let's mark it deprecated for now. |
Good idea, would you prefer to depreciate that feature (silent flag) or the options object. As there is no need for the object at all in with depreciation, however technically we are depreciating just the silent flag. |
I think it would prefer to deprecate silent flag only, because I introduce new option in #380 😉 |
fa9df67
to
50c1478
Compare
/ping @yyx990803 @ktsn |
- Now Dev tools supports filtering silent option is no longer required - Added depreciation warning - Updated unit and type tests
50c1478
to
948857d
Compare
@@ -78,8 +78,16 @@ class Store { | |||
handler(payload) | |||
}) | |||
}) | |||
if (!options || !options.silent) { | |||
this._subscribers.forEach(sub => sub(mutation, this.state)) | |||
this._subscribers.forEach(sub => sub(mutation, this.state)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should not only provide deprecation message but also retain the silent flag's behavior until next major release.
this._subscribers.forEach(sub => sub(mutation, this.state)) | ||
|
||
if ( | ||
process.env.NODE_ENV !== 'production' && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As vuex currently does not resolve NODE_ENV
check on the build time, we should not introduce it?
This line would cause runtime error if we use Vuex from <script>
tag.
Or can we update build configuration for it?
@ktsn to be honest this feature is rarely used, so probably no need to check env variable. Will modify to still retain existing silent functionality if @yyx990803 agrees |