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

babel-generator: Ensure ASCII-safe output for string literals #4478

Merged
merged 1 commit into from Sep 8, 2016

Conversation

Projects
None yet
7 participants
@mathiasbynens
Copy link
Contributor

commented Sep 8, 2016

Q A
Bug fix? no
Breaking change? no
New feature? no
Deprecations? no
Spec compliancy? no
Tests added/pass? yes
Fixed tickets #4477
License MIT
Doc PR none

Fixes #4477.

@mathiasbynens mathiasbynens referenced this pull request Sep 8, 2016

Closed

Ensure ASCII-safe output #4477

2 of 3 tasks complete
@codecov-io

This comment has been minimized.

Copy link

commented Sep 8, 2016

Current coverage is 88.32% (diff: 100%)

No coverage report found for master at 2b4e49d.

Powered by Codecov. Last update 2b4e49d...59e8a7d

@@ -4,6 +4,7 @@
'\x20';
"\n\r";
"😂";
`😂`;

This comment has been minimized.

Copy link
@mathiasbynens

mathiasbynens Sep 8, 2016

Author Contributor

I’ve added this test to indicate that this patch only handles string literals and not template literals. (Regular expression literals are already taken care of through our use of regexpu.)

@mathiasbynens mathiasbynens changed the title transform-es2015-literals: Ensure ASCII-safe output babel-generator: Ensure ASCII-safe output for string literals Sep 8, 2016

// ensure the output is ASCII-safe
let val = jsesc(node.value, {
quotes: t.isJSX(parent) ? "double" : this.format.quotes,
wrap: true
});

This comment has been minimized.

Copy link
@mathiasbynens

mathiasbynens Sep 8, 2016

Author Contributor

The use of jsesc also allows us to get rid of the hacky parts below (i.e. remove double quotes, unescape double quotes, escape single quotes, add single quotes).

This comment has been minimized.

Copy link
@hzoo

hzoo Sep 8, 2016

Member

Nice to remove all that 😄

This comment has been minimized.

Copy link
@kangax

kangax Oct 7, 2016

Member

Fun fact: I updated Babel to 6.17 at Facebook and it broke one of our (PHP) parsers and the entire transform pipeline as a result :D After tedious investigation, I think this is the culprit. When someone uses "×" in our codebase (within custom tags) it's then transformed to '×' which Babel represented as '\u00D7' before. Now jsesc turns it into '\xD7'. Apparently PHP parser doesn't understand the latter. ¯_(ツ)_/¯

This comment has been minimized.

Copy link
@mathiasbynens

mathiasbynens Oct 7, 2016

Author Contributor

@kangax Which PHP parser is that?

This comment has been minimized.

Copy link
@kangax

kangax Oct 7, 2016

Member

Dear god... Tracked it down further to json_decode returning null whenever there's \x in a string.

This comment has been minimized.

Copy link
@mathiasbynens

mathiasbynens Oct 7, 2016

Author Contributor

That actually makes sense as JSON only supports \uXXXX-style escape sequences. (As a hotfix, you could use jsesc(string, { json: true }) here in Babel to enforce JSON output but really, it sounds like you need another parser since the intention is to parse JavaScript strings rather than JSON strings.)

@@ -15,6 +15,7 @@
"babel-runtime": "^6.9.0",
"babel-types": "^6.14.0",
"detect-indent": "^3.0.1",
"jsesc": "^1.3.0",

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Sep 8, 2016

Member

Looks like this is Node >= 4 so I don't think we can land this at the moment. We all agree that we want to bump to >= 4 AFAIK, but we won't be able to do it until we bump our major version, which we haven't really had the opportunity to plan yet.

This comment has been minimized.

Copy link
@danez

danez Sep 8, 2016

Member

AFAIK it looks to me that 1.x supports versions < 4 and 2.0 only supports 4+
mathiasbynens/jsesc@e212a67
But haven't tested.

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Sep 8, 2016

Member

Ahh my mistake then, didn't compare major versions.

This comment has been minimized.

Copy link
@mathiasbynens

mathiasbynens Sep 8, 2016

Author Contributor

@danez is right. I’ve used jsesc@1.3.0 because it supports older versions of Node.js.

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Sep 8, 2016

Member

Thanks!

This comment has been minimized.

Copy link
@hzoo

hzoo Sep 8, 2016

Member

Also CI was passing on node 0.10 😄

@hzoo hzoo added the PR: Polish 💅 label Sep 8, 2016

@hzoo hzoo added this to the Next Patch milestone Sep 8, 2016

@hzoo

This comment has been minimized.

Copy link
Member

commented Sep 8, 2016

Actually, it would be nice if the changelog included new/removed deps

@hzoo hzoo merged commit b9919bd into babel:master Sep 8, 2016

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected.
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +11.67% compared to 1445dad
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

novemberborn added a commit to novemberborn/babel-plugin-files that referenced this pull request Oct 17, 2016

Fix test
Babel now generates ASCII-safe output for string literals, see
<babel/babel#4478>.

@jeromew jeromew referenced this pull request Oct 26, 2016

Closed

Rewrite using babel #21

@wenbing
Copy link

left a comment

make it unreadable!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.