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

turn on ascii_only for issue with emoji and regex #2596

Merged
merged 1 commit into from
Jun 27, 2017

Conversation

viankakrisna
Copy link
Contributor

@viankakrisna viankakrisna commented Jun 22, 2017

Fixing #2488
Verified the changes by cloning https://github.com/crabicode/create-react-app-emoji and manually editing react-scripts/config/webpack.config.prod.js
Before:
screen shot 2017-06-23 at 3 01 48 am
After:
screen shot 2017-06-23 at 3 01 08 am

@gaearon
Copy link
Contributor

gaearon commented Jun 22, 2017

Do we need to report a bug to Uglify?

@viankakrisna
Copy link
Contributor Author

The question is why did they have that option in the first place and not turned on by default? I'll try to dig more on that.

@viankakrisna
Copy link
Contributor Author

mishoo/UglifyJS#171 I think they do that because it affects bundle size.

@viankakrisna
Copy link
Contributor Author

it also suggests that <meta charset="utf8"/> on index.html would fix that. we have <meta charset="utf-8"> on public/index.html

@gaearon
Copy link
Contributor

gaearon commented Jun 22, 2017

If we change it to remove dash does it work? Then I'd prefer to do that.

@viankakrisna
Copy link
Contributor Author

it doesn't work when I remove the dash
screen shot 2017-06-23 at 3 33 56 am
making it uppercase also not working
screen shot 2017-06-23 at 3 35 14 am
There are 32 issues related to ascii_only and special characters in uglifyjs2 repo https://github.com/mishoo/UglifyJS2/search?p=1&q=ascii_only&type=Issues&utf8=%E2%9C%93
So I think it's better to turn it on. If we want to file issue, maybe to turn it on by default?

@gaearon gaearon added this to the 1.0.8 milestone Jun 26, 2017
@gaearon gaearon merged commit 41f2013 into facebook:master Jun 27, 2017
@gaearon
Copy link
Contributor

gaearon commented Jun 27, 2017

Thanks!

This was referenced Jun 27, 2017
romaindso pushed a commit to romaindso/create-react-app that referenced this pull request Jul 10, 2017
wmonk referenced this pull request in wmonk/create-react-app-typescript Aug 7, 2017
morgs32 pushed a commit to BrickworkSoftware/create-react-app that referenced this pull request Sep 1, 2017
@lock lock bot locked and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants