feat(warn): warn when an existing property starting with $ is not pro… #8214
Conversation
…perly accessed Closes vuejs#8213
| target | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
Should we do the same thing when we come across properties prefixed with _?
There was a problem hiding this comment.
that's true, I thought there is an eslint warning for it. Let me check again
There was a problem hiding this comment.
I added that as well but I wonder if it is correct. I'm not sure what babel helpers are we talking about in 8e1478e
There was a problem hiding this comment.
Why not proxying both? I changed all mixins in a big project to respect this rule on Style Guide (https://vuejs.org/v2/style-guide/#Private-property-names-essential) and now when I am trying to pass it to a component prop it gets there undefined because it is not being proxied
There was a problem hiding this comment.
to prevent conflicts. We cannot check all existing properties (which would be ideal), for conflicts, so we test that instead.
Nowadays, if you want to create plugins that inject custom properties, using Symbols is probably the safest way
There was a problem hiding this comment.
Yet the style guide recommends it, I think this issue should be given some thought, either change the rule for private property names or proxy everything, (maybe giving a warning in development process). Not proxying this is removing functionality and forcing developers that follow the recommended style guide find workarounds because of limited functionality to prevent others who may not read the documentation and override vue properties.
src/core/instance/proxy.js
Outdated
| if (!has && !isAllowed) { | ||
| warnNonPresent(target, key) | ||
| if (key in target.$data) warnStartingWithDollar(target, key) |
There was a problem hiding this comment.
Does it have to be some property starting with preserved prefixes here? It feels a little weird to me that after these checks we concluded that the property starts with _ or $.
There was a problem hiding this comment.
if the key is in $data it is not proxified, then it can only be starting with those 2 characters. That's what I was thinking
There was a problem hiding this comment.
Proxying properties starting with $_ would be an option
src/core/instance/proxy.js
Outdated
| @@ -24,6 +24,16 @@ if (process.env.NODE_ENV !== 'production') { | |||
| ) | |||
| } | |||
|
|
|||
| const warnStartingWithDollar = (target, key) => { | |||
There was a problem hiding this comment.
Something like warnPreservedPrefix?
| template: `<div>{{ $data.$a }}</div>` | ||
| }).$mount() | ||
| expect(`Property or method "$a" is not defined`).not.toHaveBeenWarned() | ||
| expect(`Property or method "$a" is not defined`).not.toHaveBeenWarned() |
There was a problem hiding this comment.
These two expects are identical, is that accidental?
…perly accessed
Closes #8213
What kind of change does this PR introduce? (check at least one)
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing applications:
The PR fulfills these requirements:
devbranch for v2.x (or to a previous version branch), not themasterbranchfix #xxx[,#xxx], where "xxx" is the issue number)If adding a new feature, the PR's description includes:
Other information: