Use raw value of JSXText and JSXAttribute by rattrayalex · Pull Request #5256 · babel/babel

rattrayalex · 2017-02-02T01:29:34Z

Q	A
Patch: Bug Fix?	yes
Major: Breaking Change?	maybe?
Minor: New Feature?	no
Deprecations?	no
Spec Compliancy?	I think so
Tests Added/Pass?	Few tests added, which pass
Fixed Tickets	Fixes babel/babylon#316 , Fixes babel/babylon#271
License	MIT
Doc PR	no
Dependency Changes	babylon is changed as well

Depends on babel/babylon#344 (see that PR for description; happy to elaborate here if desired).

babel-bot · 2017-02-02T01:39:42Z

Hey @rattrayalex! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

rattrayalex · 2017-02-02T01:41:03Z

Tests are failing b/c they're reading from the wrong version of Babylon; not sure how that is best addressed.

xtuc · 2017-02-02T07:27:51Z

Thanks for your PR 👍

If tests are green locally, that's fine for now. We need to wait for babel/babylon#344.

danez

Thanks for your PR but this is wrong, as JSX is not escaped as JS but as XHTML. That is the reason why the raw value is currently not present, so that we can escape the raw xhtml value with jsesc in babel when the jsx tag is converted to a StringLiteral. This will currently break I think and jsx->js is probably more used than jsx->jsx.

Having no raw for now is correct.

rattrayalex · 2017-02-02T18:11:36Z

@danez can you provide an example test case?

rattrayalex · 2017-02-02T19:10:42Z

Ahh, I think I understand... I essentially need to add tests with the transform-react-jsx plugin enabled, correct? And verify that, eg, React.createElement('div', {}, '\\w') is output instead of React.createElement('div', {}, '\w') ?

If so, didn't quite grasp the nature of the challenge at hand, apologies.

Judging by @kittens 's rants about the Literal node type, we'll need to do something like addExtra('isJSXText', true) – does that sound palatable?

EDIT: obviously not ideal, but perhaps better than removing an otherwise valid attribute from extra, which seems quite a bit hackier

rattrayalex · 2017-02-02T19:37:33Z

Another possibility may be modifying the transform-react-jsx plugin itself, probably eg; here and here. Removing (or modifying) node.extra.rawand node.extra.rawValue at that point may be more appropriate.

Thoughts?

loganfsmyth · 2017-02-02T22:33:44Z

packages/babel-generator/src/generators/jsx.js

+    const raw = node.extra && node.extra.raw;
    this.token("=");
-    this.print(node.value, node);
+    this.print(raw || node.value, node);


I know this PR has a few comments against the current implementation already, but I also want to add that this could in some cases lead to a breaking change. Say you had a transform that worked on these nodes that changed the .value. With this PR, the mutated value would be ignored in the final output since raw would exist. You'd want to use getPossibleRaw:

babel/packages/babel-generator/src/printer.js

Line 390 in 672adba

getPossibleRaw(node) {

so that value is compared to rawValue

Hmm, I'd intentionally avoided that because of the value and rawValue comparison, thinking that they'd be unequal in exactly the scenarios this PR attempts to address. But I didn't actually test that, and of course your point is valid, so I'll try to find a way to address it (may need to compare an escaped version of value or similar).

Thanks for the helpful catch @loganfsmyth !

I think you can use getPossibleRaw the same way we do for strings: https://github.com/babel/babel/blob/master/packages/babel-generator/src/generators/types.js#L139, if this PR is in fact accepted.

Cool, I'll give it a try. Thanks!

rattrayalex · 2017-02-10T01:56:43Z

Thanks for the feedback @loganfsmyth – using getPossibleRaw now.

I'm now deleting node.extra.raw from the StringLiteral as it's generated from JSXAttribute, which feels much more appropriate and fixes the issues at hand.

I also added tests explicitly for the jsx->js case.

@danez does this look better to you?

babel-bot · 2017-02-10T01:57:44Z

Hey @rattrayalex! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

rattrayalex · 2017-02-10T02:00:21Z

(as before, tests failing because they don't have the matching version of babylon, and pass locally)

rattrayalex · 2017-02-10T02:05:47Z

...el-plugin-transform-react-jsx/test/fixtures/react/should-escape-xhtml-jsxattribute/actual.js

@@ -0,0 +1,3 @@
+<div id="wôw" />;
+<div id="\w" />;
+<div id="w &lt; w" />;


would appreciate suggestions for other test-cases, as I think this is the crux of the issue (and was previously untested)

loganfsmyth · 2017-02-10T16:53:24Z

packages/babel-generator/src/generators/jsx.js

+    const raw = this.getPossibleRaw(node);
    this.token("=");
-    this.print(node.value, node);
+    this.print(raw || node.value, node);


I don't think this particular call is actually doing anything, because JSXAttribute nodes don't have a .raw, their .value property does. I'm assuming this is working entirely because StringLiteral already checks for .raw.

Ah, yes, good catch. Thanks.

loganfsmyth · 2017-02-10T16:56:22Z

packages/babel-generator/src/generators/jsx.js

 export function JSXText(node: Object) {
-  this.token(node.value);
+  const raw = this.getPossibleRaw(node);
+  this.token(raw || node.value);


Can we explicitly check === null since raw being an empty string would also cause this to use .value instead?

Thinking on this all a bit, I'm up for adding raw handling if we need it, but it seems like we should also have additional logic both here and in StringLiteral to handle encoding of .value, otherwise it's still totally possible for people to assign values to JSXText that will not be printed properly. The intention of .raw is to not lose the original formatting in the source file, if possible, but either way we don't want usage of .value without .raw to cause issues.

Can we explicitly check === null since raw being an empty string would also cause this to use .value instead?

Yep, good catch – though I assume you mean == null ?

I'll respond to the second para in a follow-up comment, since this thread is about to disappear when I push.

rattrayalex · 2017-02-13T23:13:14Z

(pulling from in-thread comment from @loganfsmyth )

Thinking on this all a bit, I'm up for adding raw handling if we need it, but it seems like we should also have additional logic both here and in StringLiteral to handle encoding of .value, otherwise it's still totally possible for people to assign values to JSXText that will not be printed properly. The intention of .raw is to not lose the original formatting in the source file, if possible, but either way we don't want usage of .value without .raw to cause issues.

I'm not quite sure what you're suggesting?

I think the scenario you're concerned about is when a plugin modifies or creates a JSXText node with an invalid value – is that correct? And that, to defend against that, the printers for JSXText and StringLiteral should ensure that the value is always safe?

babel-bot · 2017-02-13T23:14:57Z

Hey @rattrayalex! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

danez · 2017-03-16T17:32:44Z

I think what @loganfsmyth means is that even if raw is not set babel-generator should correctly create the raw value from value, so that we are not dependent on raw. Though I have no idea how we should do that, as I can't think of a solution of how we should do this correctly in all cases. Maybe you have an idea?

danez · 2017-03-16T17:33:51Z

I'm fine with this as is for 7.0.

rattrayalex · 2017-03-16T17:35:01Z

Gotcha. Thanks @danez !

danez · 2017-03-16T17:35:48Z

Can you rebase on the 7.0 branch?

rattrayalex · 2017-03-16T17:36:41Z

Will do – though specs will still fail until babel/babylon#344 has gone out on 7.0

rattrayalex · 2017-03-16T17:39:35Z

Rebased and changed target

loganfsmyth · 2017-04-04T16:05:52Z

Looks like this is causing some test failures now that it is merged. Anyone know what's up? Did we land this too early since you said it depends on Babylon?

danez · 2017-04-04T16:56:29Z

The tests are failing now because they are assuming new Babylon version. I wanted to create a new Babylon release for 7.0 but haven't gotten around. The change was bc so nothing really is broken.

This was referenced Feb 2, 2017

JSXText is missing {extra: {raw}} field babel/babylon#316

Closed

Nonbreakable space html entity is converted to a space prettier/prettier#547

Closed

xtuc added pkg: generator enhancement labels Feb 2, 2017

danez suggested changes Feb 2, 2017

View reviewed changes

loganfsmyth reviewed Feb 2, 2017

View reviewed changes

rattrayalex force-pushed the jsx-escaping branch from a882792 to 8fe77b8 Compare February 10, 2017 01:52

rattrayalex commented Feb 10, 2017

View reviewed changes

loganfsmyth reviewed Feb 10, 2017

View reviewed changes

rattrayalex mentioned this pull request Feb 13, 2017

[JSX] Do not convert html-escaped entities to their prettier forms prettier/prettier#678

Closed

rattrayalex force-pushed the jsx-escaping branch from 8fe77b8 to 262a42d Compare February 13, 2017 23:09

danez approved these changes Mar 16, 2017

View reviewed changes

danez added PR: Bug Fix 🐛 A type of pull request used for our changelog categories and removed enhancement labels Mar 16, 2017

rattrayalex mentioned this pull request Mar 16, 2017

Add extra.raw back to JSXText and JSXAttribute babel/babylon#344

Merged

rattrayalex force-pushed the jsx-escaping branch from 262a42d to abf6d58 Compare March 16, 2017 17:38

Use raw value of JSXText and JSXAttribute

abf6d58

rattrayalex changed the base branch from master to 7.0 March 16, 2017 17:39

danez merged commit 348cc5e into babel:7.0 Apr 4, 2017

yuche mentioned this pull request Jun 10, 2018

中文 placeholder 显示异常 NervJS/taro#55

Closed

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 6, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019

Uh oh!

Conversation

rattrayalex commented Feb 2, 2017

Uh oh!

babel-bot commented Feb 2, 2017

Uh oh!

rattrayalex commented Feb 2, 2017

Uh oh!

xtuc commented Feb 2, 2017

Uh oh!

danez left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rattrayalex commented Feb 2, 2017

Uh oh!

rattrayalex commented Feb 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rattrayalex commented Feb 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rattrayalex commented Feb 10, 2017

Uh oh!

babel-bot commented Feb 10, 2017

Uh oh!

rattrayalex commented Feb 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rattrayalex commented Feb 13, 2017

Uh oh!

babel-bot commented Feb 13, 2017

Uh oh!

danez commented Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danez commented Mar 16, 2017

Uh oh!

rattrayalex commented Mar 16, 2017

Uh oh!

danez commented Mar 16, 2017

Uh oh!

rattrayalex commented Mar 16, 2017

Uh oh!

rattrayalex commented Mar 16, 2017

Uh oh!

loganfsmyth commented Apr 4, 2017

Uh oh!

danez commented Apr 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

danez left a comment •

edited

Loading

rattrayalex commented Feb 2, 2017 •

edited

Loading

danez commented Mar 16, 2017 •

edited

Loading