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

Support for JSX tag namespaces <React:DOM:div /> #760

Merged
merged 1 commit into from
May 22, 2014
Merged

Support for JSX tag namespaces <React:DOM:div /> #760

merged 1 commit into from
May 22, 2014

Conversation

syranide
Copy link
Contributor

This has been suggested and discussed before: #74, #221, facebookarchive/esprima#6, etc.

But it seems to me like the discussions rather died out than a rejection of the issue. So here's for one final resurrection of the issue, with a proper implementation, using the XML-namespaces as seemed to be preferred by @jeffmo.

In my opinion, this issue points to a very pleasant but not required feature that vanilla JS has, that JSX currently lacks. To me, it also highlights a possible issue with respect to best practices for sharing components and frameworks, as this may have impact design decisions depending on the authors preference for JS or JSX. Having to locally require and assign every single component intended to be used is tedious and unnecessarily verbose. Simply being able to refer to a collection of components, as we easily can in JS, should be available to JSX as well.

// In plain JS
var MyDOM = require('MyDOM');
MyDOM.ComponentA(null);
MyDOM.ComponentB(null);

// In existing JSX
var MyDOM = require('MyDOM');
var MyDOM_ComponentA = MyDOM.ComponentA;
var MyDOM_ComponentB = MyDOM.ComponentB;
<MyDOM_ComponentA />
<MyDOM_ComponentB />

// In enhanced JSX
var MyDOM = require('MyDOM');
<MyDOM:ComponentA />
<MyDOM:ComponentB />

This brings JS and JSX to parity on this issue in my opinion, without inventing custom features or weird syntaxes, and it seems to me that this should not block any important future design decisions (that would be compatible with JSX), and it's entirely optional for the user.

Technically, the JSX implementation quotes invalid JS identifiers resulting from unusual characters in namespaces or tags (when possible), which explains the implementation of quoteNamespacedTagName.

This PR depends on facebookarchive/esprima#7 for fb-esprima, which is why the tests are failing.

All feedback and criticisms are appreciated.

Fixes #74.

@syranide
Copy link
Contributor Author

cc @vjeux

return name;
} else {
return quoteNamespacedTagName(namespace, true) +
(isValidJSIdentifier(name) ? '.' + name : "['" + name + "']");
Copy link
Collaborator

Choose a reason for hiding this comment

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

'[' + JSON.stringify(name) + ']'

would make me feel a bit better. hyphens seem a bit weird to support though.

@syranide
Copy link
Contributor Author

@spicyj Ah yes, that was copied from the existing quoteAttrName it should be safe the way it is done, but I don't know why we wouldn't use JSON.stringify regardless (committed).

As for hyphens, tag names and attributes share the same parseXJSIdentifier in esprima which explicitly supports hyphens (I assume for attributes), but React uses camelCasing so if it was up to me, we could do away with the hyphen entirely (it would be easy to keep separate character whitelists for them too if desired).

return name;
} else {
var append = isValidJSIdentifier(name) ?
'.' + name : '[' + JSON.stringify(name) + ']';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: put the else part in the next line to make it easier to read

var append = isValidJSIdentifier(name) ?
  '.' + name :
  '[' + JSON.stringify(name) + ']';

@syranide
Copy link
Contributor Author

@vjeux Thanks! All nits should have been resolved except for the error, that convention is used for other errors (in those files at least), so I'll hold off on that for moment unless you want me to correct all of them.

}

return identifier.name;
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

note: you don't need the else since you return from the if branch :) This lets you have one less level of indentation

@ghost ghost assigned jeffmo Dec 31, 2013
@syranide
Copy link
Contributor Author

@vjeux All fixed, however note that you must apply the fb-esprima PR too to be able to test it.

@chikamichi
Copy link

This looks very promising!

@martynsmith
Copy link

This would be a very cool feature to have added (I'm experiencing exactly this problem at the moment).

@jessep
Copy link

jessep commented Jan 11, 2014

I also very much want to be able to do this. Makes me sadder than a reddit user without cat pics not to have it.

@syranide
Copy link
Contributor Author

A reason not to use : and to use . instead is that : collides with potential XML use. I seem to remember someone mentioning about having to namespace some of the attributes for SVG. I think logically it makes sense to use . unless we actually lose something by not using :.

@flashjames
Copy link

We've been having the same problem for a while. We use require.js and without this PR fix we have a few choices:

a) Have really unreadable code, when using multiple components, i.e. not use JSX syntax and write it in vanilla js
b) Only have one component per file
c) Place components that need to use eachother in same file (same require js define).
d) Have lot's of boilerplate code, for example: var Todos = TodoComponents.TodoNote;
before we use

Right now we're using a mix of a),c) and d) depending on how much nestling we have of multiple components.

So big upvote on this PR request!

@vjeux
Copy link
Contributor

vjeux commented Jan 20, 2014

@jenso: Why not doing b) and having a component per file? We do that internally and it's worked very well for us.

@sophiebits
Copy link
Collaborator

Though I agree that one component per file is probably better, I don't think it's React's job to force a particular way of organizing files.

@jeffmo
Copy link
Contributor

jeffmo commented Jan 21, 2014

Namespacing would be a very useful tool and I believe those who have expressed desire for it on this thread would benefit from some form of namespacing, but I'm not sure this is the best way to deal with that need in a general sense. Specifically, I am opposed to this particular approach because it conflates "namespacing" with code organization.

If the only way to namespace your components is to put them together in a module (or nest them together within an object), then namespacing of components can quickly become at odds with the way one organizes their code -- which is a pretty unfortunate trade-off to have to make.

Consider, for example, if someone wanted to create a "common component library" (let's call it CCL for short) where it is desired that all of the components should be namspaced with "CCL". So <ccl:checkbox /> and <ccl:textinput /> (etc).

The only way the library author would be able to achieve this are:

  1. Put all of the libraries' components into one huge module that exports them all:

ccl.js

var checkbox = React.createClass({
  render: function() { ... }
});

var textinput = React.createClass({
  render: function() { ... }
});

export {
  checkbox,
  textinput
};

Obviously for a large library, this is pretty unacceptable as it doesn't allow the library author to break their code into smaller more manageable (testable/isolated) pieces. It might be reasonable for smaller libraries and codebases, but it's not a scalable way to write a component library.

  1. Place each component in its own module, then create an aggregation module that imports all of the components and exports them nested in some object:

checkbox.js

export default React.createClass({
  render: function() { ... }
});

textinput.js

export default React.createClass({
  render: function() { ... }
});

ccl.js

import checkbox from "checkbox.js";
import textinput from "textinput.js";

export default {
  checkbox: checkbox,
  textinput: textinput
};

This is MUCH more reasonable from the library author's perspective, but it pushes some issues off to the users of the library. The users can now choose how they will consume the lib, but their choice comes with trade-offs:

a) Users can import only the components they want to use in their local file, but forgo namespacing to do so:

thing-that-uses-ccl.js

import checkbox from "ccl/checkbox.js"

export default React.createClass({
  render: function() {
    return (
      <div><checkbox /></div>
    );
  }
});

b) Users can import from an aggregator module -- which includes all of the components in the namespace -- to achieve namespacing in their local file:

thing-that-uses-ccl.js

import ccl from "ccl/ccl.js";

export default React.createClass({
  render: function() {
    return (
      <div><ccl:checkbox /></div>
    );
  }
});

This second solution is still pretty unfortunate for a few reasons:

  1. It forces users of the library to make a potentially significant trade-off between namespacing and byte-size/packaging. It's practically impossible for any packaging system to identify and eliminate dead-code in this scenario (i.e. unused components) without some pretty sophisticated type tracking system.
  2. It gives users of the library two different ways to access the components...but at a cost. You can always pick a style-guide rule for your codebase, but in this case if you choose "always use namespacing" you may need to allow awkward exceptions to the rule to support perf/size-critical areas of your codebase. This isn't always the worst thing in the world (especially for codebases with a smaller team behind them -- where everyone can remember when/why the style-guide exception applies), but it is still pretty unfortunate for code-styling as a whole (and for larger codebases).
  3. It makes it a little too easy to do the wrong thing: Write all your components in a single module and export them. Why not just this one time? I only have 3 stupid-simple components in my library... Of course, if you only ever have 3 tiny components in your library, you may encounter no issues here -- but if you have more, or if you ever change your mind and decide to add more, or those 3 "tiny" modules grow to be not so tiny anymore: You're likely going to run into problems pretty quickly. Of course best practice might be to use the aggregator-module pattern even if you only have very few simple components to start with -- and I whole-heartedly agree that it's not our job to tell people which best practices they should maintain -- but it is definitely our job to consider the kinds of ways people will use our the React library and structure it to encourage better patterns where and when possible pit of success, and all that jazz)

Another note: I would like to defer the . vs : separator syntax discussion until we have a better understanding of what we want the semantic meaning for namespacing to be. Syntax is important, but its significantly less important than what it means to "namespace" a React component from a semantic perspective.

@jeffmo
Copy link
Contributor

jeffmo commented Jan 21, 2014

In summary at a higher level: If we went with this means of namespacing, it would mean namespacing is really only a feature usable by smaller teams and/or codebases. I'd really like to see us try to come up with a more scalable solution so that everyone can benefit and use them.

@syranide
Copy link
Contributor Author

@jeffmo Valid concerns, but intuitively to me it seems that by those restrictions, the only valid solution is no "namespacing". I don't see what middle-ground there is between a component, and components in an object. It also seems weird to me that this would be considered an issue when React's own DOM components are exported as React.DOM.div. If it's a good idea for React, why is it bad for everyone else?

But I truthfully respect your opinion on this, I'm not really an architecture guy, but more of a get shit done and solve it later (and then later), and it's certainly not always the right "attitude". However, I would like to put another perspective on it by saying that I think it's easier to get people to start using React if they can continue with their bad habbits and then you show them light. Rather than have React feel alien simply because it doesn't work/support the way they are used to and feels encumbering instead. JavaScript micro libraries popping up everywhere is not just because, but because people been taught that it's better (for the worse).

I would rather have 1 person using React the right way and 1 person using it the wrong way, than just 1 person using it the right way. Put up a big guide on best practices in the docs, the people who want to learn will read it and happily submit, those who don't simply aren't ready and forcing them will only push them away (that said, one should pick the right solution, but lets not artificially limit users for philosophical reasons). Right now we kind of have neither.

@sophiebits
Copy link
Collaborator

For what it's worth, including only some of the tags from React.DOM wouldn't give an appreciable size savings.

@syranide
Copy link
Contributor Author

@jeffmo PS. To me <svg.circle />, <ccl.input />, etc, actually provides meaningful context as well, without having to manually bake it into the variable name (we meet again Mr. Bad Habits user, not actually baking it into the name).

@jeffmo
Copy link
Contributor

jeffmo commented Jan 21, 2014

What if namespacing a component didn't mean "nested in an object", but instead it meant "this component claims to be a member of this namespace"?

I'm totally riffing here -- and I don't claim these ideas are well thought-out -- but just as a thought experiment:

checkbox.jsx

export default React.createClass({
  namespace: 'MyLibrary',
  render: function() { ... }
});

thing-that-uses-MyLibrary.jsx

import checkbox from "checkbox.js";

export default React.createClass({
  render: function() {
    return (
      <div><MyLibrary:checkbox /></div>
    );
  }
});

(which desugars to)

thing-that-uses-MyLibrary.desugared.js

import checkbox from "checkbox.js"

export default React.createClass({
  render: function() {
    return (
      React.DOM.div(
        null, // namespace
        null, // props
        checkbox(
          'MyLibrary', // namespace
          null // namespace
        )
      )
    );
  }
});

I think this would address the issue of "true namespacing" where the components dictate what namespace they belong to (not the location of the code), and it would mean that namespacing could probably still be enforced statically with some tooling and a module system that supports statically-analyzable imports/exports.

It would not necessarily make life easier for those who use nested objects as a means of namespacing -- though I would imagine we could make some simple tweaks and utilities to make that not horribly complicated:

MyLibrary.not-a-module.js

var MyLibrary = ReactUtils.namespacify('MyLibrary', {
  checkbox: React.createClass({
    render: function() { ... }
  }),

  textinput: React.createClass({
    render: function() { ... }
  })
};

ReactUtils.js

function namespaceify(namespace, object) {
  for (var key in object) {
    var nestedThing = object[key];
    if (isAReactComponentDescriptor(nestedThing)) {
      nestedThing.namespace = namespace;
    } else if (typeof nestedThing === 'object' && nestedThing !== null) {
      namespaceify(namespace + ':' + key, nestedThing);
    }
  }
}

@jeffmo
Copy link
Contributor

jeffmo commented Jan 21, 2014

The namespace could be verified at runtime (or even statically without types if you have the right setup -- a statically analyzable module system + tooling that supports it + string literals for the namespace entry in the createClass() call) to guarantee that what you're calling for is correct.

It also serves as documentation for the reader of the code.

And it leaves the door open to any module system rather than just a particular style

Thoughts here?

@jeffmo
Copy link
Contributor

jeffmo commented Jan 21, 2014

It would also not solve the issue of having to extract items from a nested object into a local binding:

var checkbox = MyLibrary.checkbox;

React.createClass({
  render: function() { 
    return (
      <div><MyLibrary:checkbox /></div>
    );
  }
});

But this is necessary in just about any module system anyway (you have to put require() or import at the top somewhere) -- which doesn't seem to be a deal-breaker for people who use those things... I am inclined to suggest a perhaps controversial opinion of my own though and say that using a module system is truly the best practice; and so while we should totally support non-best-practices, I'm not so sure that we should add feature-sets that make trade-offs in their favor.

That said, as I mentioned to @spicyj on IRC, if we want to talk about opening up JSX to accept more than just an Identifier for a tag name -- I might feel a little better about going full-monty and opening it up to accept full AssignmentExpressions rather than just special-casing Identifier and MemeberExpression somewhat arbitrarily.

I think if we do that though, we should have a "more endorsed" means of namespacing first so that it's clear that we discourage the use of object-nesting for namespacing purposes. People could still do <MyLibrary.checkbox /> if they want to disregard best-practice -- but for people who agree with and wish to conform to something a little more flexible, they'll have a better more robust option to start with (rather than having to settle with the nested-object form until we get namespaces set up -- and then go refactor).

@jeffmo
Copy link
Contributor

jeffmo commented Feb 3, 2014

Yea, I used html:, but I imagine most people would probably knock it down to something short like h: as you suggest.

I have no problem with supporting a default namespace + runtime disambiguation though

@zpao
Copy link
Member

zpao commented Mar 13, 2014

Ok, we're going to do member expressions. I think we're going to start with supporting <Foo.Bar.Baz /> - not entirely sure what to do about <Foo["Bar"].Baz /> (@jeffmo?)

If you're still interested in picking this back up @syranide, I think you're best situated to get back into it.

@syranide
Copy link
Contributor Author

@zpao <Foo.Bar.Baz /> is basically already covered by this PR (namespacing syntax, not a member expression), just need to change : to . and restore XML-NS, or if @jeffmo has any preference when it comes to the implementation itself.

What about <Foo["Bar"].Baz />? Intuitively I don't see why we feel the need to support it other than for edge-case Google Closure Compiler support (I assume?). If that's the case, not saying it's a good idea but <Foo."Bar".Baz /> could be a possibility, but really, it would be better for people to just export it correctly.

@percyhanna
Copy link

If adding support for <Foo["Bar"].Baz />, will it indirectly add support for any valid JS identifier/expression? e.g. <Foo[myVar] />?

@vjeux
Copy link
Contributor

vjeux commented Mar 15, 2014

Supporting arbitrary JS expression may be tricky. What happens if you write

<Foo[myVar()]>
  ...
</Foo[myVar()]>

Are the two expressions executed? What if they don't match? Lots of corner cases

@zpao
Copy link
Member

zpao commented Apr 1, 2014

@syranide, let's make it so it's only possible to have dot format. Forget I said anything about <Foo["Bar"].Baz />.

@syranide
Copy link
Contributor Author

syranide commented Apr 6, 2014

@zpao Sorry for the delay, been out sick for a bit. I'll polish up the PR to use . and restore the old namespacing code (that would be my interpretation of what's been said).

@syranide
Copy link
Contributor Author

syranide commented Apr 7, 2014

@jeffmo Hmm, do you want this implemented "properly" as namespaces (the way it's currently implemented), or should I basically extend the valid symbols with . instead? If we go with namespacing, by what name should I refer to it? "namespace" I would say is owned by XML-NS.

@jeffmo
Copy link
Contributor

jeffmo commented Apr 8, 2014

IIRC I think we agreed to go with non-computed member expressions -- so no more colon.
so:

// success
<MyStuff.MyThing>
</MyStuff.MyThing>

// error, because open/closing tags don't match statically
var myThing = MyStuff.MyThing;
<MyStuff.MyThing>
</myThing>

@syranide
Copy link
Contributor Author

syranide commented Apr 9, 2014

@jeffmo Right, but how do you want it implemented? It's currently implemented as a nested structure called namespace or something like that (div -> DOM -> React I think), but that's for : which is XML-NS, so I need to restore that so we can have the old warnings I assume.

So the question is, should we just use the same nested structure still, div -> DOM -> React or React -> DOM -> div, what do we refer to them as memberExpression -> memberExpression? Or go the cheap route and just add . to the list of allowed symbols?

@jeffmo
Copy link
Contributor

jeffmo commented Apr 13, 2014

Ah -- In this case we should update the parser to parse either Identifier or MemberExpression

@syranide
Copy link
Contributor Author

syranide commented May 7, 2014

@jeffmo Updated according to our discussions (you basically coded it, haha). Just shout if there's anything you want changed. It depends on facebookarchive/esprima#7 to function, but it should not break without it.

@zpao As per the nudge :)

@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!

@syranide
Copy link
Contributor Author

@jeffmo Just reminding in-case you forgot about this PR since the esprima PR landed a while ago (facebookarchive/esprima#7).

When done you can also tick complete in https://github.com/facebook/react/wiki/Projects#jsx-namespacing. :)

jeffmo pushed a commit that referenced this pull request May 22, 2014
Support for JSX tag namespaces <React:DOM:div />
@jeffmo jeffmo merged commit 9ef6156 into facebook:master May 22, 2014
@jeffmo
Copy link
Contributor

jeffmo commented May 22, 2014

welp.

@nullobject
Copy link

Nice work 👍

@bradparks
Copy link

woot woot! thanks guys!

@syranide syranide deleted the jsxns branch May 23, 2014 08:07
@chikamichi
Copy link

Thank you :) 🍰

@jordwalke
Copy link
Contributor

👍

@ghostandthemachine
Copy link

👍

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.

Allow namespacing in component names in JSX