Skip to content

Remove length restriction from JSX entities, and ignore Object.prototype#14327

Merged
nicolo-ribaudo merged 5 commits intobabel:mainfrom
nicolo-ribaudo:jsx-entities
Mar 6, 2022
Merged

Remove length restriction from JSX entities, and ignore Object.prototype#14327
nicolo-ribaudo merged 5 commits intobabel:mainfrom
nicolo-ribaudo:jsx-entities

Conversation

@nicolo-ribaudo
Copy link
Copy Markdown
Member

@nicolo-ribaudo nicolo-ribaudo commented Mar 3, 2022

Q                       A
Fixed Issues? Fixes #14316
Patch: Bug Fix? Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

The second/third commits are for a bug I found while fixing the main one.

@nicolo-ribaudo nicolo-ribaudo added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: parser area: jsx labels Mar 3, 2022
@babel-bot
Copy link
Copy Markdown
Collaborator

babel-bot commented Mar 3, 2022

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51411/

let val;

if (code === charCodes.underscore) {
if (code === charCodes.underscore && allowNumSeparator !== "bail") {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The invalid JSX syntax &#0_0; is recoverable: we can either pass allowNumSeparator: false and refine the error message of NumericSeparatorInEscapeSequence, or pass a context string so we can throw different errors. Though personally I think html entities are just like escape sequences in JS.

Copy link
Copy Markdown
Member Author

@nicolo-ribaudo nicolo-ribaudo Mar 4, 2022

Choose a reason for hiding this comment

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

It's not invalid, it's rendeded as-is and not as an HTML entity. I'll add a test.

/* allowNumSeparator */ "bail",
);
if (
codePoint &&
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
codePoint &&
codePoint !== null &&

� is valid html entity.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This might be another one to spec in JSX btw, because there’s likely divergence between implementations. There are a bunch of different things not allowed by XML/HTML/markdown (such as \0 or lone surrogates)

if (code === charCodes.underscore && allowNumSeparator !== "bail") {
const prev = this.input.charCodeAt(this.state.pos - 1);
const next = this.input.charCodeAt(this.state.pos + 1);
if (allowedSiblings.indexOf(next) === -1) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not related to this PR: these errors should be thrown only when allowNumSeparator is true, otherwise the errors might be confusing.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'll open a PR when this is merged.

@nicolo-ribaudo nicolo-ribaudo merged commit 4bb9b89 into babel:main Mar 6, 2022
@nicolo-ribaudo nicolo-ribaudo deleted the jsx-entities branch March 6, 2022 09:00
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jun 6, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area: jsx outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: Bug Fix 🐛 A type of pull request used for our changelog categories

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Loosen the char length limits of HTMLCharacterReference in JSX

5 participants