facebook / jsx

The JSX specification is a XML-like syntax extension to ECMAScript.
http://facebook.github.io/jsx/
1.96k stars 132 forks source link

Fix attribute strings spec to match implementations #133

Closed strager closed 2 years ago

strager commented 2 years ago

According to my understanding of the JSX specification, the following code should define one attribute whose name is attr and whose value is hello/*"*/world:

function MyComponent() {
  return <div attr="hello/*"*/world" />;
}

However, I found no implementation which parses the code this way. All implementations treat /* as part of the JSXSingleStringCharacters rule rather than the beginning of a block comment.

Disallow parsing Comment and WhiteSpace rules between elements of JSXSingleStringCharacters, etc. This makes the specification match implementations.

Implementations tested, all of which agree with the modified grammar:

facebook-github-bot commented 2 years ago

Hi @strager!

Thank you for your pull request.

We require contributors to sign our Contributor License Agreement, and yours needs attention.

You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

facebook-github-bot commented 2 years ago

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

wooorm commented 2 years ago

The text contains:

Whitespace and Comments

JSX uses the same punctuators and braces as ECMAScript. WhiteSpace, LineTerminators and Comments are generally allowed between any punctuators.

There are several other cases where the text is theoretically ambiguous (e.g., comments or whitespace inside inside identifiers?).

But to me I don’t see any reason to assume that comments or whitespace would be allowed in a group of characters?


It seems a bit too over the top to add that big bold capitalized “NO WHITESPACE OR COMMENT” everywhere?

nicolo-ribaudo commented 2 years ago

If we want to make the spec "correct" as an extension to the ECMAScript spec, the solution is to use JSXDoubleStringCharacters :
: instead of JSXDoubleStringCharacters :
 (two :s), to make it part of the "lexical grammar" which disallows comments/spaces: https://tc39.es/ecma262/#sec-lexical-and-regexp-grammars

strager commented 2 years ago

There are several other cases where the text is theoretically ambiguous (e.g., comments or whitespace inside inside identifiers?).

JSXIdentifier excludes whitespace and comments explicitly.

It seems a bit too over the top to add that big bold capitalized “NO WHITESPACE OR COMMENT” everywhere?

I was following the lead of JSXIdentifier.

If we want to make the spec "correct" as an extension to the ECMAScript spec, the solution is to use JSXDoubleStringCharacters :
: instead of JSXDoubleStringCharacters :
 (two :s), to make it part of the "lexical grammar" which disallows comments/spaces: https://tc39.es/ecma262/#sec-lexical-and-regexp-grammars

This sounds like a great solution!

wooorm commented 2 years ago

@strager Sorry, you are right that JSXIdentifier does! JSXText doesn’t.