Skip to content

fix #8862: cannot watch unicode properties like 'a.中文' #8925

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

Closed
wants to merge 6 commits into from

Conversation

derek-li
Copy link

@derek-li derek-li commented Oct 10, 2018

Hi all at Vue!

This pull request resolves the issue regarding watchers not watching unicode properties like 'a.中文'.

My method for resolving this case is to throw a warning when non-alphanumeric, $, or _ characters are detected. My reasoning behind throwing a warning instead of supporting special characters is that I think supporting special characters would promote poor naming habits that could only make code more difficult to read and understand.

I'm pretty new to open source so any feedback would be awesome and please let me know if you'd like to see any changes or improvements!

Link to issue: #8862

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

@posva
Copy link
Member

posva commented Oct 10, 2018

This is not fixing the issue it says to be fixing 🤔

@derek-li
Copy link
Author

@posva The issue was that Vue was throwing an incorrect warning regarding watchers only watching dot-delimited paths.

I added an error to show that Vue does not support using special characters in property naming.
For reasons I stated above, I feel that this is the appropriate solution to this issue rather than supporting bad coding habits that would lead to code that is more difficult to read and understand.

However, if this is the route that you think is better I'm open to making another pull request to address this issue. What do you think?

@yyx990803
Copy link
Member

I think that's a decent workaround, not technically a fix, but I agree with the approach to the original issue.

@@ -80,13 +80,21 @@ export default class Watcher {
} else {
this.getter = parsePath(expOrFn)
if (!this.getter) {
if (this.getter === false) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't like that we are relying on magic return values (false) here, as the logic is separated in two places. Also the check is warning only so it's better to place it right here inside a if (process.env.NODE_ENV !== 'production') { ... } block, using an inline regexp.

Copy link
Author

Choose a reason for hiding this comment

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

@yyx990803 Thanks for taking the time to review this PR.

I've gone ahead and made the commits to stop relying on random return values. I also tried your suggestion with having the check in an if (process.env.NODE_ENV !== 'production') { ... } block, but felt that it was unclear to have two regex checks in different places (watcher and parsePath) and did not resolve your comment on the logic being separated, so I grouped them together.

Thanks again and please let me know what you think!

@yyx990803
Copy link
Member

Note this may become invalid if we merge #8666, I'm still debating whether we should support it.

@yyx990803
Copy link
Member

Closed via #8666

@yyx990803 yyx990803 closed this Feb 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants