Skip to content

Allow namespacing in refs #430

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

Closed
shripadk opened this issue Oct 16, 2013 · 7 comments
Closed

Allow namespacing in refs #430

shripadk opened this issue Oct 16, 2013 · 7 comments

Comments

@shripadk
Copy link
Contributor

It'll be great if one could namespace references to backing instances returned from render.

Silly example:

var Editor = React.createClass({
  componentDidMount: function() {
    if(this.refs.hasOwnProperty('syntax') {
      for(var lang in this.refs.syntax) {
        var editor = this.refs.syntax[lang];
        editor.prettify().colorize();
      }
    }

    this.refs.plain.prettify();
  },
  render: function() {
    return (
      <div id='content'>
        <TextEditor ref='syntax.javascript' />
        <TextEditor ref='syntax.html' />
        <TextEditor ref='syntax.css' />
        <TextEditor ref='plain' />
      </div>
    );
  }
});
@chenglou
Copy link
Contributor

You can do it so: this.refs['syntax.' + etc]
This gets the job done with slight modifications.

@shripadk
Copy link
Contributor Author

Can you modify my example specifically to show how it can be done with existing implementation. Your case adds tight coupling with the render code. So if I, for instance (sometime in the future), remove all the TextEditor instances that have ref='syntax.*' it'll throw an error. I'll have to then modify code in all places that depend on this kind of tight coupling (in this specific case componentDidMount). But I may not be able to remove them as some other code still has references to TextEditor instances which I cannot remove. This makes it too complicated.

I assumed that if this issue #221 is being considered, then might as well implement namespacing for references.

@chenglou
Copy link
Contributor

I don't think #221 is being considered now. I was saying you could loop through them and check for the syntax. word but this isn't optimal. The thing is, you shouldn't be looping through refs to manipulate the components. What are you trying to do?

@zpao
Copy link
Member

zpao commented Oct 21, 2013

Nothing has happened on #221 recently, but I wouldn't count it out yet.

I'm not convinced we want to make this sort of change to refs. By using a string we don't need to have complicated logic and we don't need to actually parse it, just use it. If somebody want's to use a period in their string, why not let them. Also, we don't really have the guarantee that the period is a real separator, ref="version.1.0" might actually be intended to be parsed to this.refs.version['1.0'] but it's actually parsed to this.refs.version['1']['0']. and then there's the problem of ref="version.1" which would get parsed to this.refs.version['1'], and you can no longer add anything else under that, but it wouldn't be totally obvious. By keeping it simple we keep surprises to a minimum.

The difference between refs and components (#221) is that we don't have any implied semantics for <Component/>, in fact simple namespacing there follows naturally from everything we have claimed, namely that if it's not an HTML component, we just assume it's a function in scope, since that what we transform to - Component(null). In fact there's nothing stopping you from writing the transformed JS yourself and using namespaces - MyApp.Component(null). For props, we very much imply that anything between quotes is a string and will stay a string.

So based on that, I'm inclined to wontfix this...

I would actually suggest maybe going in the opposite direction and storing a list of refs in an object (maybe namespace that), then iterating over that object instead of the refs, and seeing if that value exists in this.refs. Something like this (with 2 options...):

var SYNTAX_REFS = [
  'syntax.javascript',
  'syntax.html',
  'syntax.css',
  'syntax.c++'
]

// or maybe
var REFS = {
  syntax: {
    html: 'syntax.html',
    ...
    'c++': 'syntax.c++'
  }
}

var Editor = React.createClass({
  componentDidMount: function() {
    SYNTAX_REFS.forEach(function(ref) {
      if (ref in this.refs) {
        var editor = this.refs[ref];
        editor.prettify().colorize();
      }
    }, this);

    // OR if using REFS

    for (lang in REFS.syntax) {
      var ref = REFS.syntax[lang];
      if (ref in this.refs) {
        var editor = this.refs[ref];
        editor.prettify().colorize();
      }
    }

    this.refs.plain.prettify();
  },
  render: function() {
    return (
      <div id='content'>
        <TextEditor ref='syntax.javascript' />
        <TextEditor ref='syntax.html' />
        <TextEditor ref='syntax.css' />
        <TextEditor ref='plain' />
      </div>
    );
  }
});

@sophiebits
Copy link
Collaborator

+1 @zpao.

@jordwalke
Copy link
Contributor

Good points @zpao. Another place where namespacing has been requested is in props:

<div style.backgroundColor="black" style.width={10} />

Which would transform into:

div({style:{backgroundColor:'black', width: 10}});

That sounds like a good idea at first as well, but may also suffer from the same problems @zpao mentioned.

@zpao
Copy link
Member

zpao commented Nov 4, 2013

@shripadk I'm going to wontfix this based on the above. I appreciate the discussion though and love hearing ideas people have for improving the way React works. Keep them coming :)

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

No branches or pull requests

5 participants