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

#631 - Silent "Avoid mutating a prop directly" warning when updating prop #688

Merged
merged 4 commits into from
Jun 14, 2018

Conversation

cdbkr
Copy link
Contributor

@cdbkr cdbkr commented Jun 7, 2018

This PR aims to solve #631

By default, Vue is throwing warnings when setProps is used/invoked, due to changes to observers and "Vue.config.silent" is false by default https://vuejs.org/v2/api/#silent

Since "silent" suppress all warnings and logs, it would be great to reduce the scope just to the desired functions/blocks.

Please, take this as an initial proposal and discuss if it makes sense to adopt this strategy and feel free to point missing implementation.

@eddyerburgh
Copy link
Member

eddyerburgh commented Jun 8, 2018

Thanks for the PR!

I think this is fine.

The only negative I can see is that it would silence any true errors, so when somebody is trying to change a prop that doesn't exist. But at the moment, there are so many errors that you can't tell the difference anyway.

@cdbkr
Copy link
Contributor Author

cdbkr commented Jun 8, 2018

Thanks to you!

Your point makes total sense to me and we don't want to suppress valid errors (which can prevent bugs).

Anyway, when changing props which don't exist, aren't we expecting an exception instead of a console error?

it('throws error if component does not include props key', () => {

I added a test case locally and the exception is thrown when changing the not existing prop (when silentWarnings is set to true).

Also, is there any missing part which would use the same logic? (if yes, maybe it would be great to discuss/consider decorators instead)

@eddyerburgh
Copy link
Member

Ah great, I forgot we had that functionality!

I can't see any other methods currently where we need to set the config to silent.

@cdbkr
Copy link
Contributor Author

cdbkr commented Jun 8, 2018

Great, if you agree, I'm going to include that test case covering the scenario you described (in case of logic changes) and write some documentation regarding the introduced property "silentWarnings".

@eddyerburgh
Copy link
Member

Yep sounds good

Copy link
Member

@eddyerburgh eddyerburgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍

I'll release this evening

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.

2 participants