Fixes React isCompatTag validator accepting leading dash character #7164

claudiopro · 2018-01-06T11:43:22Z

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

Fixed regexp test in isCompatTag React validator allowing leading dash '-' characters.

Was:

/^[a-z]|-/.test(tagName)

Is:

// Must start with a lowercase ASCII letter
/^[a-z]/.test(tagName)

nicolo-ribaudo · 2018-01-06T14:05:07Z

Quoting the react docs: [1]

When an element type starts with a lowercase letter, it refers to a built-in component like <div> or
<span> and results in a string 'div' or 'span' passed to React.createElement. Types that start with a capital letter like <Foo /> compile to React.createElement(Foo) and correspond to a component defined or imported in your JavaScript file.

Things like aA, a-b-c and a1 should all be threated like built-in components. So I think that the correct regular expression is just /^[a-z]/

This is also the reason why some tests are failing.

[1] https://reactjs.org/docs/jsx-in-depth.html#user-defined-components-must-be-capitalized

bvaughn

Some thoughts?

bvaughn · 2018-01-06T16:09:20Z

packages/babel-types/test/validators.js

+      assert(t.react.isCompatTag("-div") === false);
+      // assert(t.react.isCompatTag("-span") === false);
+      // assert(t.react.isCompatTag("-a") === false);
+      // assert(t.react.isCompatTag("-input") === false);


Why are these commented out?

Ha, my mistake! I forgot to uncomment before committing 😅

bvaughn · 2018-01-06T16:09:38Z

packages/babel-types/test/validators.js

+      // assert(t.react.isCompatTag("-a") === false);
+      // assert(t.react.isCompatTag("-input") === false);
+    });
+  });


Test for trailing dash might be nice. That's a use case that fails currently (but would be fixed by this PR) I think.

Thanks for taking the time to review on a Saturday @bvaughn 😄 Let me update the PR.

Let me ask this though as I'm not clear if we're on the same page: trailing dashes are legal for custom elements, i.e. <input-/> is a legal custom element name, or am I misreading the spec?

When @hzoo tags me, I come running. (In case there's Mario Kart.)

bvaughn · 2018-01-06T16:10:53Z

packages/babel-types/src/validators/react/isCompatTag.js

@@ -1,4 +1,5 @@
 // @flow
 export default function isCompatTag(tagName?: string): boolean {
-  return !!tagName && /^[a-z]|-/.test(tagName);
+  // Must start with 'a'-'z', can have a dash '-', but must finish with 'a'-'z'
+  return !!tagName && /^[a-z]+(-?[a-z]+)?$/.test(tagName);


This pattern would reject custom element names with more than one hyphen (eg my-custom-tag).

I think this would work though?
/^[^-]([\-a-z]*[^-])*$/

(['a', 'ab', 'abc', 'a-b', 'a-b-c']) .map(tag => /^[^-]([\-a-z]*[^-])*$/.test(tag)) .join(','); // true,true,true,true,true (['-a', 'a-', '-', '-a-']) .map(tag => /^[^-]([\-a-z]*[^-])*$/.test(tag)) .join(','); // false,false,false,false

Edit: This would fail "a--b" though. Could write a simpler regex if you only used it for strings with 3+ characters. 1-2 character strings could be validated with a simple /[a-z]+/ check. Not sure how bulletproof this regex needs to be.

By the way, glancing at the spec it seems like a much broader range of characters than just a-z are allowed for custom elements (eg <emotion-😍>) 😆 Not sure if we want to try to support them all. Probably not?

I don't have enough context to answer that. 😄 My guess would be "probably not".

claudiopro · 2018-01-06T17:11:42Z

I decided to back out changes to validate the element name and only fix the validator not to accept a leading dash, and add unit tests 😄 The regex can be further complicated if needs to be to match the spec.

Thanks @nicolo-ribaudo @bvaughn for the feedback!

babel-bot · 2018-01-06T17:17:09Z

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

bvaughn

Looks like I accidentally approved earlier when I only meant to "comment" 😄 Clumsy.

I don't know that I have enough context to approve of this change with confidence. The regex I mentioned on a previous comment seems closer to ideal, even though not perfect, but maybe there are perf considerations?

I'll defer to someone who has more context. 😄

claudiopro · 2018-01-06T17:28:45Z

@bvaughn +1 on perf concerns. Also, it seems there are other checks happening before the validator as malformed JSX tags like <-div> are rejected by the REPL: https://tinyurl.com/y7wfomel

nicolo-ribaudo · 2018-01-06T17:43:30Z

They are rejected by Babylon (the parser). Anyway, I'd still accept this PR if it removes the - from the test for the first character.

bvaughn · 2018-01-06T17:57:28Z

Also, it seems there are other checks happening before the validator as malformed JSX tags like <-div> are rejected by the REPL: https://tinyurl.com/y7wfomel

Agreed. Leading "-" fails in master now. Trailing "-" doesn't (but still won't if this change lands).

hzoo · 2018-01-06T18:43:20Z

Yeah I didn't think this change was necessary and was never reported before (forgot to mention), just wanted to ping someone on the react team for review

Ya if it errors in the parser already, I think

babel/packages/babylon/src/plugins/jsx/index.js

Line 517 in b93184e

if (isIdentifierStart(code)) {

then this check is more for babel to pick a string or identifier, new tests are welcome thou!

bvaughn · 2018-01-06T19:18:07Z

react-domwill fail at runtime ifownerDocument.createElement(type)` fails, so I think it's kind of up to Babel how it wants to do compile-time tag name verification.

IMO it's nice if JSX compilation fails earlier for things that are obviously wrong (like leading ~~or trailing~~ slashes) because it's a better developer experience.

Edit: Actually, it looks like trailing slashes are accepted, so clearly I don't know much about this topic:

document.createElement('a-')
// <a-></a->

bvaughn · 2018-01-06T19:27:49Z

What about using /^[^-](\-?[a-z]+)*-?$/?

function test(tag) {
  if (/^[^-](\-?[a-z]+)*-?$/.test(tag)) {
    console.log(`creating tag "${tag}"`);
    document.createElement(tag);
  } else {
    console.log(`skipping tag "${tag}"`);
  }
}

// valid tags
(['a', 'ab', 'abc', 'a-b', 'a-b-c', 'a-', 'a-b-']).forEach(test);

// creating tag "a"
// creating tag "ab"
// creating tag "abc"
// creating tag "a-b"
// creating tag "a-b-c"
// creating tag "a-"
// creating tag "a-b-"

// invalid tags
(['-a', '-', '-a-', 'a--b']).forEach(test);

// skipping tag "-a"
// skipping tag "-"
// skipping tag "-a-"
// skipping tag "a--b"

nicolo-ribaudo · 2018-01-06T19:50:40Z

Uppercase letters and numbers are also allowed. e.g. spaN, or h3.
The isCompatTag function should just be "Given this valid* JSX tag, should it become a string or an identifier?". To answer to this question, checking wether the first character is lowercase or not is enough.

* Babel plugins assume that the AST given to them is valid. The check against invalid ASTs should either be in Babylon or in babel-types.

claudiopro · 2018-01-06T19:55:28Z

@nicolo-ribaudo my thoughts exactly, but I relate with @bvaughn, I'm falling prey to nerd sniping too 😅

bvaughn · 2018-01-06T20:07:53Z

Uppercase letters and numbers are also allowed. e.g.

In JSX, where all tag names are strings, host components (eg div, span) and custom components (eg MyButton) are distinguished by the casing of the first letter. I assume the intent here is to give helpful warnings as early as possible rather than to be 100% correct or cover all cases. That being said, I've weighed in enough here. I have only partial contenxt and no strong opinions 😄 so I'm bowing out now. 👋

Jessidhia · 2018-01-09T07:13:43Z

packages/babel-types/test/validators.js

+      assert(t.react.isCompatTag("plastic-button")); // ascii letters
+      assert(t.react.isCompatTag("math-α")); // non-latin chars
+      assert(t.react.isCompatTag("img-viewer2")); // numbers
+      assert(t.react.isCompatTag("emotion-😍")); // emoji


Not sure if anything besides /^[a-z][a-z-]*$/ is supposed to work, but I don't see why ban it unless the JSX spec forbids it.

Checked the spec; the spec actually says nothing about compat tags at all; just that the identifier (JSXIdentifier) is basically the same thing as IdentifierName but it also allows -.

I assume that that part is enforced by babylon instead of isCompatTag so this seems sufficient.

Emojis in source code are cool 😆

Jessidhia · 2018-01-09T08:36:22Z

Test failure is unrelated (yarnpkg timeout?)

nicolo-ribaudo · 2018-01-09T08:41:16Z

Yes, it's failing often today (and yesterday). I restarted it but it failed again. I think we can marge this PR anyway since it doesn't affect Babylon?

claudiopro · 2018-01-09T13:35:07Z

Thank you all 😊

hzoo assigned bvaughn and gaearon and unassigned bvaughn and gaearon Jan 6, 2018

hzoo requested review from bvaughn and gaearon January 6, 2018 15:07

bvaughn approved these changes Jan 6, 2018

View reviewed changes

Fixes React isCompatTag validator accepting leading dash character

4c512ba

claudiopro force-pushed the master branch from 36e8f2f to 4c512ba Compare January 6, 2018 17:06

bvaughn reviewed Jan 6, 2018

View reviewed changes

Jessidhia approved these changes Jan 9, 2018

View reviewed changes

nicolo-ribaudo approved these changes Jan 9, 2018

View reviewed changes

nicolo-ribaudo merged commit ce420ba into babel:master Jan 9, 2018

hzoo added area: tests PR: Internal 🏠 A type of pull request used for our changelog categories labels Jan 9, 2018

dependabot-preview bot mentioned this pull request Mar 14, 2018

Bump @babel/register from 7.0.0-beta.37 to 7.0.0-beta.41 nju33/yosuga#189

Closed

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes React isCompatTag validator accepting leading dash character #7164

Fixes React isCompatTag validator accepting leading dash character #7164

claudiopro commented Jan 6, 2018 •

edited

Loading

nicolo-ribaudo commented Jan 6, 2018 •

edited

Loading

bvaughn left a comment

bvaughn Jan 6, 2018

claudiopro Jan 6, 2018

bvaughn Jan 6, 2018

claudiopro Jan 6, 2018 •

edited

Loading

bvaughn Jan 6, 2018

bvaughn Jan 6, 2018 •

edited

Loading

bvaughn Jan 6, 2018

claudiopro Jan 6, 2018 •

edited

Loading

bvaughn Jan 6, 2018

claudiopro commented Jan 6, 2018 •

edited

Loading

babel-bot commented Jan 6, 2018 •

edited

Loading

bvaughn left a comment

claudiopro commented Jan 6, 2018

nicolo-ribaudo commented Jan 6, 2018 •

edited

Loading

bvaughn commented Jan 6, 2018

hzoo commented Jan 6, 2018 •

edited

Loading

bvaughn commented Jan 6, 2018 •

edited

Loading

bvaughn commented Jan 6, 2018 •

edited

Loading

nicolo-ribaudo commented Jan 6, 2018 •

edited

Loading

claudiopro commented Jan 6, 2018

bvaughn commented Jan 6, 2018 via email

Jessidhia Jan 9, 2018

Jessidhia Jan 9, 2018

nicolo-ribaudo Jan 9, 2018

Jessidhia commented Jan 9, 2018

nicolo-ribaudo commented Jan 9, 2018

claudiopro commented Jan 9, 2018

Fixes React isCompatTag validator accepting leading dash character #7164

Fixes React isCompatTag validator accepting leading dash character #7164

Conversation

claudiopro commented Jan 6, 2018 • edited Loading

nicolo-ribaudo commented Jan 6, 2018 • edited Loading

bvaughn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

claudiopro Jan 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn Jan 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

claudiopro Jan 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

claudiopro commented Jan 6, 2018 • edited Loading

babel-bot commented Jan 6, 2018 • edited Loading

bvaughn left a comment

Choose a reason for hiding this comment

claudiopro commented Jan 6, 2018

nicolo-ribaudo commented Jan 6, 2018 • edited Loading

bvaughn commented Jan 6, 2018

hzoo commented Jan 6, 2018 • edited Loading

bvaughn commented Jan 6, 2018 • edited Loading

bvaughn commented Jan 6, 2018 • edited Loading

nicolo-ribaudo commented Jan 6, 2018 • edited Loading

claudiopro commented Jan 6, 2018

bvaughn commented Jan 6, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jessidhia commented Jan 9, 2018

nicolo-ribaudo commented Jan 9, 2018

claudiopro commented Jan 9, 2018

claudiopro commented Jan 6, 2018 •

edited

Loading

nicolo-ribaudo commented Jan 6, 2018 •

edited

Loading

claudiopro Jan 6, 2018 •

edited

Loading

bvaughn Jan 6, 2018 •

edited

Loading

claudiopro Jan 6, 2018 •

edited

Loading

claudiopro commented Jan 6, 2018 •

edited

Loading

babel-bot commented Jan 6, 2018 •

edited

Loading

nicolo-ribaudo commented Jan 6, 2018 •

edited

Loading

hzoo commented Jan 6, 2018 •

edited

Loading

bvaughn commented Jan 6, 2018 •

edited

Loading

bvaughn commented Jan 6, 2018 •

edited

Loading

nicolo-ribaudo commented Jan 6, 2018 •

edited

Loading