Skip to content

Conversation

poteto
Copy link
Member

@poteto poteto commented Jun 20, 2024

Stack from ghstack (oldest at bottom):

This PR extends the previous logic added in #29141 to also account for
other kinds of non-ascii characters such as \n. Because these control
characters are individual special characters (and not 2 characters \
and n) we match based on unicode which was already being checked for
non-Latin characters.

This allows control characters to continue to be compiled equivalently
to its original source if it was provided in a JsxExpressionContainer.
However note that this PR does not convert JSX attributes that are
StringLiterals to JsxExpressionContainer, to preserve the original
source code as it was written.

Alternatively we could always emit a JsxExpressionContainer if it was
used in the source and not try to down level it to some other node
kind. But since we already do this I opted to keep this behavior.

Partially addresses #29648.

[ghstack-poisoned]
Copy link

vercel bot commented Jun 20, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 20, 2024 11:12pm

poteto added a commit that referenced this pull request Jun 20, 2024
This PR extends the previous logic added in #29141 to also account for
other kinds of non-ascii characters such as `\n`. Because these control
characters are individual special characters (and not 2 characters `\`
and `n`) we match based on unicode which was already being checked for
non-Latin characters.

This allows control characters to continue to be compiled equivalently
to its original source if it was provided in a JsxExpressionContainer.
However note that this PR does not convert JSX attributes that are
StringLiterals to JsxExpressionContainer, to preserve the original
source code as it was written.

Alternatively we could always emit a JsxExpressionContainer if it was
used in the source and not try to down level it to some other node
kind. But since we already do this I opted to keep this behavior.

ghstack-source-id: 42a80bb
Pull Request resolved: #29997
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jun 20, 2024
poteto added a commit that referenced this pull request Jun 20, 2024
This PR extends the previous logic added in #29141 to also account for
other kinds of non-ascii characters such as `\n`. Because these control
characters are individual special characters (and not 2 characters `\`
and `n`) we match based on unicode which was already being checked for
non-Latin characters.

This allows control characters to continue to be compiled equivalently
to its original source if it was provided in a JsxExpressionContainer.
However note that this PR does not convert JSX attributes that are
StringLiterals to JsxExpressionContainer, to preserve the original
source code as it was written.

Alternatively we could always emit a JsxExpressionContainer if it was
used in the source and not try to down level it to some other node
kind. But since we already do this I opted to keep this behavior.

Partially addresses #29648.

ghstack-source-id: 42a80bb
Pull Request resolved: #29997
[ghstack-poisoned]
poteto added a commit that referenced this pull request Jun 20, 2024
This PR extends the previous logic added in #29141 to also account for
other kinds of non-ascii characters such as `\n`. Because these control
characters are individual special characters (and not 2 characters `\`
and `n`) we match based on unicode which was already being checked for
non-Latin characters.

This allows control characters to continue to be compiled equivalently
to its original source if it was provided in a JsxExpressionContainer.
However note that this PR does not convert JSX attributes that are
StringLiterals to JsxExpressionContainer, to preserve the original
source code as it was written.

Alternatively we could always emit a JsxExpressionContainer if it was
used in the source and not try to down level it to some other node
kind. But since we already do this I opted to keep this behavior.

Partially addresses #29648.

ghstack-source-id: 965cf50
Pull Request resolved: #29997
[ghstack-poisoned]
poteto added a commit that referenced this pull request Jun 20, 2024
This PR extends the previous logic added in #29141 to also account for
other kinds of non-ascii characters such as `\n`. Because these control
characters are individual special characters (and not 2 characters `\`
and `n`) we match based on unicode which was already being checked for
non-Latin characters.

This allows control characters to continue to be compiled equivalently
to its original source if it was provided in a JsxExpressionContainer.
However note that this PR does not convert JSX attributes that are
StringLiterals to JsxExpressionContainer, to preserve the original
source code as it was written.

Alternatively we could always emit a JsxExpressionContainer if it was
used in the source and not try to down level it to some other node
kind. But since we already do this I opted to keep this behavior.

Partially addresses #29648.

ghstack-source-id: e5f3ca7
Pull Request resolved: #29997
[ghstack-poisoned]
poteto added a commit that referenced this pull request Jun 20, 2024
This PR extends the previous logic added in #29141 to also account for
other kinds of non-ascii characters such as `\n`. Because these control
characters are individual special characters (and not 2 characters `\`
and `n`) we match based on unicode which was already being checked for
non-Latin characters.

This allows control characters to continue to be compiled equivalently
to its original source if it was provided in a JsxExpressionContainer.
However note that this PR does not convert JSX attributes that are
StringLiterals to JsxExpressionContainer, to preserve the original
source code as it was written.

Alternatively we could always emit a JsxExpressionContainer if it was
used in the source and not try to down level it to some other node
kind. But since we already do this I opted to keep this behavior.

Partially addresses #29648.

ghstack-source-id: 552bd44
Pull Request resolved: #29997
```

### Eval output
(kind: ok) <div><span>A E</span><span>나은</span><span>Sathya</span></div> No newline at end of file
Copy link
Member Author

Choose a reason for hiding this comment

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

note that these still render correctly and no extra escaping was added

[ghstack-poisoned]
poteto added a commit that referenced this pull request Jun 20, 2024
This PR extends the previous logic added in #29141 to also account for
other kinds of non-ascii characters such as `\n`. Because these control
characters are individual special characters (and not 2 characters `\`
and `n`) we match based on unicode which was already being checked for
non-Latin characters.

This allows control characters to continue to be compiled equivalently
to its original source if it was provided in a JsxExpressionContainer.
However note that this PR does not convert JSX attributes that are
StringLiterals to JsxExpressionContainer, to preserve the original
source code as it was written.

Alternatively we could always emit a JsxExpressionContainer if it was
used in the source and not try to down level it to some other node
kind. But since we already do this I opted to keep this behavior.

Partially addresses #29648.

ghstack-source-id: 032700c
Pull Request resolved: #29997
[ghstack-poisoned]
poteto added a commit that referenced this pull request Jun 20, 2024
This PR extends the previous logic added in #29141 to also account for
other kinds of non-ascii characters such as `\n`. Because these control
characters are individual special characters (and not 2 characters `\`
and `n`) we match based on unicode which was already being checked for
non-Latin characters.

This allows control characters to continue to be compiled equivalently
to its original source if it was provided in a JsxExpressionContainer.
However note that this PR does not convert JSX attributes that are
StringLiterals to JsxExpressionContainer, to preserve the original
source code as it was written.

Alternatively we could always emit a JsxExpressionContainer if it was
used in the source and not try to down level it to some other node
kind. But since we already do this I opted to keep this behavior.

Partially addresses #29648.

ghstack-source-id: ecc61c9
Pull Request resolved: #29997
@react-sizebot
Copy link

react-sizebot commented Jun 20, 2024

Comparing: 0f56841...53436c3

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.66 kB 6.66 kB = 1.82 kB 1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 497.93 kB 497.93 kB = 89.26 kB 89.26 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.67 kB 6.67 kB = 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 502.75 kB 502.75 kB = 89.96 kB 89.96 kB
facebook-www/ReactDOM-prod.classic.js = 597.17 kB 597.17 kB = 105.33 kB 105.33 kB
facebook-www/ReactDOM-prod.modern.js = 571.52 kB 571.52 kB = 101.27 kB 101.27 kB
test_utils/ReactAllWarnings.js Deleted 62.88 kB 0.00 kB Deleted 15.69 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
test_utils/ReactAllWarnings.js Deleted 62.88 kB 0.00 kB Deleted 15.69 kB 0.00 kB

Generated by 🚫 dangerJS against 53436c3

Copy link
Member

@josephsavona josephsavona left a comment

Choose a reason for hiding this comment

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

Nice!!!!

@poteto poteto merged commit 53436c3 into gh/poteto/17/base Jun 21, 2024
poteto added a commit that referenced this pull request Jun 21, 2024
This PR extends the previous logic added in #29141 to also account for
other kinds of non-ascii characters such as `\n`. Because these control
characters are individual special characters (and not 2 characters `\`
and `n`) we match based on unicode which was already being checked for
non-Latin characters.

This allows control characters to continue to be compiled equivalently
to its original source if it was provided in a JsxExpressionContainer.
However note that this PR does not convert JSX attributes that are
StringLiterals to JsxExpressionContainer, to preserve the original
source code as it was written.

Alternatively we could always emit a JsxExpressionContainer if it was
used in the source and not try to down level it to some other node
kind. But since we already do this I opted to keep this behavior.

Partially addresses #29648.

ghstack-source-id: ecc61c9
Pull Request resolved: #29997
@poteto poteto deleted the gh/poteto/17/head branch June 21, 2024 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants