Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Fix: casing of unicode characters #694

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
armano2 wants to merge 6 commits into vuejs:master from armano2:unicode-casing

Conversation

Copy link
Contributor

@armano2 armano2 commented Dec 2, 2018
edited
Loading

@armano2 armano2 self-assigned this Dec 2, 2018
Copy link
Contributor Author

armano2 commented Dec 4, 2018

What i should do with this than? Remove support for utf-8 and add rule to ban non a-zA-Z characters instead?

i feel like its base on preference, some ppl may want to use non ascii characters like #688

Copy link
Member

I think that:

  • the utilities have to behave as same as Vue.js's.
  • the rule which disallows non-ascii upper case characters in names would be nice because it can cause unexpected behavior.

@armano2 armano2 changed the title (削除) Fix casing of unicode characters (削除ここまで) (追記) Fix: casing of unicode characters (追記ここまで) Dec 8, 2018
Copy link
Member

@michalsnik michalsnik left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Good work @armano2. Now we can also add rule to prevent usage of non-ascii characters

Copy link
Member

@mysticatea or @ota-meshi can you also take a look at the code here? I would appreciate second pair of eyes before merging it :)

Copy link
Member

My opinion is not changed since #694 (comment). I meant that our utilities should behave as same as https://github.com/vuejs/vue/blob/dev/src/shared/util.js. That doesn't have special handling for non-ASCII characters.

Copy link
Contributor Author

armano2 commented Dec 31, 2018
edited
Loading

@mysticatea what should happen with non A-Za-z0-9: characters than?

current implementation is removing them,

const invalidChars = /[^a-zA-Z0-9:]+/g

anyway new implementation is working better even without unicode characters

i fixed also issue with assert.equal(converter(' foo Bar '), 'fooBar') in camelcase fooBar instead of FooBar

Copy link
Member

@armano2 Why does it remove those?

Copy link
Contributor Author

armano2 commented Dec 31, 2018

no clue, it was copied from vue :)

Copy link
Contributor Author

armano2 commented Dec 31, 2018
edited
Loading

i will replace it with

.replace(/[\-!#$%^&*()_+~`"'<>,./?\[\]{}\s\r\n\v\t]+/gu, ' ')

Copy link
Contributor Author

armano2 commented Dec 31, 2018

and now it's ignoring unknown (non ascii) characters, and we will have to add new rule to warn about usage of non ascii characters.

Copy link
Member

no clue, it was copied from vue :)

In that case, I think we should keep it as is. My concern is that static analysis gets different behavior to Vue.js compiler and overlooks errors.

Copy link
Contributor Author

armano2 commented Dec 31, 2018

@mysticatea but in vue its used in different context https://github.com/vuejs/vue/blob/a7658e03a16dc507f0abeba41aee705f773727d0/src/core/util/options.js#L260
and from what i see it got removed anyway

Copy link
Member

mysticatea commented Dec 31, 2018
edited
Loading

My question in #694 (comment) was "why remove?". But https://github.com/vuejs/vue/blob/a7658e03a16dc507f0abeba41aee705f773727d0/src/core/util/options.js#L260 is not removal. So I should ask again, why does it remove those? If the removal is different behavior to Vue.js compiler, we should not remove those characters.

Copy link
Contributor Author

armano2 commented Dec 31, 2018
edited
Loading

that's why i changed it now, we are removing now only invalid characters

\-!#$%^&*()_+~`"'<>,./?\[\]{}\s\r\n\v\t

those characters can't be used as attribute names and property names.

Copy link
Member

mysticatea commented Dec 31, 2018
edited
Loading

Why does remove invalid characters? I think that we should handle invalid names as invalid names because this is an error checker. It should not hide errors. If Vue.js compiler removes the invalid characters, yes, we should follow that. But if not, we should follow that.

Copy link
Contributor Author

armano2 commented Jan 3, 2019

we have to somehow determine when word ends, and those are characters on which we split text, vue compile is never removing invalid characters, it's going to be runtime error..

Copy link
Contributor Author

armano2 commented Jan 11, 2019

@mysticatea than should i change this to only check whitespaces? /\s/ug

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ota-meshi ota-meshi ota-meshi left review comments

+2 more reviewers

@mysticatea mysticatea mysticatea left review comments

@michalsnik michalsnik michalsnik approved these changes

Reviewers whose approvals may not affect merge requirements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /