Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not consider namespaces when checking for DOM #2638

Merged
merged 1 commit into from May 9, 2020

Conversation

@yacinehmito
Copy link
Contributor

@yacinehmito yacinehmito commented May 9, 2020

When testing whether a component is a DOM element, we select the first portion of the name up until : or ., then tests for ^[a-z]|-.

There are many problems with this:

  • It doesn't test the name but the namespace, which I don't think is what was intended
  • In effect it only tests the first character; not the full tag EDIT: Irrelevant
  • It allows for tags that start with - (Related to this)
  • In JSX it is not possible to be a DOM element and be in a namespace as well
  • : is not an allowed character

It may be a dangerous change, but the tests are still passing and locally it is "more correct".

UPDATE: So this triggers a "chain reaction" in jsx-pascal-case, which if followed solves #1334.

@yacinehmito yacinehmito force-pushed the yacinehmito:dont-handle-namespaces branch from f82d053 to 2453bf0 May 9, 2020
code: '<testComponent />'
}, {
code: '<test_component />'
}, {
Comment on lines 32 to 35

This comment has been minimized.

@yacinehmito

yacinehmito May 9, 2020
Author Contributor

I don't know why these have been considered as valid in the first place. They are not in pascal case.

Is the reasoning that the problem is that they start with a lower case, and thus are wrong to begin with? I can see why we wouldn't want to let jsx-pascal-case trigger an error in that situation.

This comment has been minimized.

@yacinehmito

yacinehmito May 9, 2020
Author Contributor

This is exactly why: 1e17259

This comment has been minimized.

@ljharb

ljharb May 9, 2020
Collaborator

Yes, lowercase components are HTML elements, not custom ones.

@yacinehmito
Copy link
Contributor Author

@yacinehmito yacinehmito commented May 9, 2020

: is not an allowed character

About that, I saw references to the colon is a couple of places.
My current understanding, which stems from this comment, is clashing with what I see in the code. 🤔

@yacinehmito
Copy link
Contributor Author

@yacinehmito yacinehmito commented May 9, 2020

I performed more tests and found this bit of code that explains it: https://github.com/babel/babel/blob/c3a5bf1ff5bd0df437bc20752464d08674d011d3/packages/babel-types/src/validators/react/isCompatTag.js#L2

I'll revert to (almost) the same RegExp and will add a comment.

@yacinehmito
Copy link
Contributor Author

@yacinehmito yacinehmito commented May 9, 2020

@ljharb This is ready for review.

@ljharb
ljharb approved these changes May 9, 2020
@ljharb ljharb force-pushed the yacinehmito:dont-handle-namespaces branch from f545193 to ab28224 May 9, 2020
@ljharb ljharb merged commit ab28224 into yannickcr:master May 9, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.001%) to 97.618%
Details
@yacinehmito yacinehmito deleted the yacinehmito:dont-handle-namespaces branch May 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants