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

Remove JSX tag whitelist; use case instead #1551

Closed
wants to merge 1 commit into from

Conversation

sophiebits
Copy link
Collaborator

With this, tags that start with a lowercase letter are assumed to be built-in and have React.DOM prepended; uppercase tags are assumed to be user-defined and are left as-is.

Not sure if we want to do this but perhaps we do!

With this, tags that start with a lowercase letter are assumed to be built-in and have React.DOM prepended; uppercase tags are assumed to be user-defined and are left as-is.
@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@sophiebits sophiebits changed the title Remove JSX tag whitelist; use case instead Remove JSX tag whitelist; test case instead May 18, 2014
@sophiebits sophiebits changed the title Remove JSX tag whitelist; test case instead Remove JSX tag whitelist; use case instead May 18, 2014
@syranide
Copy link
Contributor

👍 related #1539

@syranide
Copy link
Contributor

If we go this route, it might make sense to separate the two in the parser already. I.e, something like XJSDOMNodeName and XJSComponentName, would allow us to apply separate parsing rules and warn accordingly.

@sophiebits sophiebits changed the title Remove JSX tag whitelist; use case instead [RFC] Remove JSX tag whitelist; use case instead May 19, 2014
@syranide
Copy link
Contributor

Btw, an argument that might lend some legitimacy to this is; HTML tags use lower-case and hyphen exclusively with the exception of SVG, the bastard child that also features case-sensitive camelCased tags and attributes, but the first char is always lower-case. Web components which are undeniably coming and fight for the same namespace as JSX, must also use lower-case and at least one hyphen.

So basically, anything that starts with a lower-case char might be reserved (by the standards or web components) outside of our control, and all work pretty much the same way (DOM nodes/elements). However, there are zero valid HTML tags that start with an upper-case char, meaning that we have that entire namespace for ourselves, no definitions required, no possible conflicts.

This would also mean that we can apply different parsing rules, it doesn't make sense to use member expressions for HTML tags, it also doesn't make sense to allow hyphens for JSX tags but we want member expressions (@jeffmo right? :P). We can enforce both at parse time.

What we end up with is razorsharp, zero config, division between the DOM (lower-case) and React's happy-fun-land (upper-case). As @jeffmo pointed out, it wreaks of convention smell and I agree, but it does work out fantastically well in the end for something that might otherwise really be a rather sticky situation.

It's also a naming convention that, at least for me, feels natural in React and is something I have self-imposed from the start.

@benjamn
Copy link
Contributor

benjamn commented May 20, 2014

I'm a big fan of this idea.

@zpao
Copy link
Member

zpao commented May 27, 2014

I personally am not a big fan of this idea. It's not entirely uncommon to pass a component type around and then construct it later. (var comp = this.props.component; do something with <comp ...>)

@syranide
Copy link
Contributor

@zpao Not saying you're wrong, but... var Comp = this.props.component; do something with <Comp ...>?

Perhaps the most important thing to keep in-mind, if we want to support web components, what do we do? Do we envision ReactComponentComponents actually being web components in the future? If so it makes sense not to do this perhaps. If we don't believe that; unless we find a non-obtuse way of dealing with web components we may be leaving a lot of users in the cold if they have to register and require them.

@sebmarkbage sebmarkbage added Flux and removed Flux labels Sep 29, 2014
@sebmarkbage
Copy link
Collaborator

I like this. I think that it's the only sane way to do this. I think that @jeffmo is the only one against it at this point. Not sure how to unblock.

Web components are unrelated since they're not ambiguous (requires a dash).

@syranide
Copy link
Contributor

@sebmarkbage Here you go 🔨 hehe. On a more serious note, I generally agree with @jeffmo on this, but it's a very neat solution to a very tricky problem unless we're going a different route (i.e. explicitly "wrap" each web-component as a react-component).

@sophiebits sophiebits added this to the 0.12 milestone Oct 3, 2014
@sophiebits sophiebits changed the title [RFC] Remove JSX tag whitelist; use case instead Remove JSX tag whitelist; use case instead Oct 3, 2014
@sophiebits
Copy link
Collaborator Author

We agreed today that we'll do this; I'll rebase when I get a chance.

@syranide
Copy link
Contributor

syranide commented Oct 3, 2014

👍

@zpao
Copy link
Member

zpao commented Oct 3, 2014

Are we going to do this before 0.12?

Edit: ah, saw the milestone.

@sebmarkbage
Copy link
Collaborator

Yes

@@ -62,7 +61,7 @@ function visitReactTag(traverse, object, path, state) {
'Namespace tags are not supported. ReactJSX is not XML.');
}

var isFallbackTag = FALLBACK_TAGS.hasOwnProperty(nameObject.name);
var isFallbackTag = /^[a-z]/.test(nameObject.name);
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this also apply it for member expressions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I need to test though it sounds like @sebmarkbage is moving some stuff around here anyway now that #2287 is in. We decided that tags with - and : are always treated as native tags (i.e., strings) and member expressions (with .) are always JS references, regardless of capitalization.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sound 👍

PS. Although I'd check for member expression and not for ..

@sophiebits
Copy link
Collaborator Author

@sebmarkbage did this in c4658c1.

@zpao
Copy link
Member

zpao commented Oct 15, 2014

Removing "accepted" so my bookmarked search for things I need to merge internally works right. (and just found out I can add is:merged and that would have worked too... I'm learning)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants