Riot: Confusing tag instance properties: which are supposed to be html nodes, which tag instances?

[juodumas]

Tag instances are populated with these properties:

• this.root - Node
• this.parent - Tag
• this.tags - Object with custom tag names as keys and Array of Tag or Tag as values
• this[value-of-name-attr] - Node in riot2, Node or Tag (for custom elements) in riot3
• this.tags[value-of-name-attr] - Tag for custom elements in riot2, undefined in riot3

http://plnkr.co/edit/p1I2rQiD4MpUofd9PC18?p=preview

I find this confusing. Maybe riot3 could have better defined and documented apis for accessing child tag instances and element nodes? While I am not sure what to propose, I think that exposing both (named nodes, tags and all child tags) is very useful when working with custom tags, but it would be nice to have a more intuitive api. Btw., riot3 behavior feels logical after thinking about it, but I think it will be very confusing for newcomers.

• [ ] Question
• [ ] Bug
• [x] Discussion
• [ ] Feature request
• [ ] Tip
• [ ] Enhancement
• [ ] Performance

[GianlucaGuarini]

I agree I could be confusing, I thought to add the ref property to cache all the Nodes instances without polluting the tag properties. What do you think guys?

cc: @tipiirai @aMarCruz @cognitom @rsbondi @rogueg

[GianlucaGuarini]

my concern is that this can be a big breaking change and all the riotjs ecosystem will be damaged by this decision. I can imagine that all the videos/tutorials/articles produced will be deprecated for such a small change

[juodumas]

So in this discussion we should also talk about how much stuff riot3 can break. Changes in the next branch will already break lots of custom tags (just look at my plnkr above) - I am concerned about compatibility here.

[rogueg]

Yeah, I found the refs really confusing as well, which prompted the change in next. I'd describe it like this:

• Any DOM node with id or name gets attached to the tag with id or name as the key.
  <div id='foo' /> => parent.foo = DOM element
• Custom tags work the same, except you get a reference to the tag, rather than the DOM node.
  <my-tag name='bar' /> => parent.bar = Tag(my-tag)
• The tags collection contains every child tag, keyed by the type of tag.
  <my-tag name='bar' /> => parent.tags['my-tag'] = Tag(my-tag)

The goal was to making tag references more predictable, and easier for newcomers to remember. You're right that it will require some code changes:

parent.bar._tag becomes just parent.bar
parent.bar becomes parent.bar.root
parent.tags.bar is now undefined

Looking at the online material, the common case seems to be referencing inputs, and that behavior hasn't changed.

[juodumas]

@rogueg: sorry, but the changes you describe look like just another shade of confusing :)
Neither riot2 nor current riot3 is intuitive in this regard, both behaviors require quite a bit of familiarity before they are understandable.

Paraphrasing your description, here is what we currently have in master:

• Any DOM node with id or name gets attached to the tag with id or name as the key.
  
  <div id='foo' /> => parent.foo = DOM element
• All custom child tags are added to the tags object, keyed by the type of tag, its id or name
  
  <my-tag name='bar' /> => parent.tags['my-tag'] = parent.tags.bar = Tag(my-tag)

I find it easier to explain the old behavior. I only find it confusing that id is preferred over name in riot2, but I guess this can be fixed without breaking compatibility.

Btw., riot-mui would break with riot3: https://github.com/kysonic/riot-mui/blob/master/src/material-elements/material-combo/material-combo.tag#L29

[rogueg]

Well, perhaps both ways take a bit of learning. I think your description of master is subtly off, because
<my-tag name='bar' /> => parent.tags['my-tag'] = undefined

In your original plunker, there are two child-tag elements, but this.tags['child-tag'] only points to one of them for riot 2. To me, that's very unexpected. In riot 3, it's an array with both child-tags.

[juodumas]

@rogueg: you are right, I missed the part where named custom tags aren't added to this.tags[tagName]. So back were I started: both behaviors can be confusing.

But still, can't we think of something without introducing breaking changes?
Maybe just fix inconsistencies:

1. Add element nodes marked with id and name to the tag instance.
2. Add named tag instances to this.tags and append to this.tags[tagName].
3. Document that instances of named tags can be accessed via this.tags.name (and should not be accessed via this.name._tag).

Or break it hard: this.rootNode, this.parent, this.nodes, this.tags, no named elements directly in the tag instance.

Which behavior do others prefer? master or next (keeping in mind that the latter breaks compatibility)?

[nodefish]

I'm a fan of the "hard break" approach you just described. It seems much cleaner than having everything in the tag instance, which is quite messy. It is 3.0.0 after all, and I think replacing references isn't a huge refactor in most projects.

A 3.0.0 migration guide could probably fit into 1 paragraph and would address any concerns in this regard, imho.

[aMarCruz]

Please make it simple... only one of this.tags[] or this.tags.name and no DOM elements in riot tags, we have querySelector.

[nodefish]

I'm not 100% clear about this, but I suspect querySelector isn't a perfect substitution, because you might want to do manipulations on the virtual DOM before the nodes are mounted onto the real DOM, and the way to do that is to use the automatically bound named elements, as discussed here #1233. In that scenario, I believe querySelector would return undefined. Please correct me if I'm wrong, as I haven't entirely wrapped my head around the inner workings of the vdom vs real dom and tag lifecycle.

I've also experienced a hard to re-produce behavior where the DOM isn't 100% mounted in the 'mount' event, and document.getElementById would return undefined. I can't seem to recreate this, and it only ever happened on iOS for me, so I have yet to report it. Named elements would alleviate this too.

[GianlucaGuarini]

I have updated the demo using the latest alpha release http://plnkr.co/edit/bd0R6RG9LXT1f2mXC8bV?p=preview I think there are still some issues

[rsbondi]

this.child as HTMLElement does not make a whole lot of sense, it would be the same as this.tags.child.root

so here is the way I see it

{
  "this.child": "Don't care, probably can go away",
  "this.tags.child": "Tag",
  "this.tags['child-tag']": "Array",
  "this.input": "HTMLInputElement",
  "this.tags['input']": "undefined",
  "this.parent": "Tag",
  "this.root": "HTMLElement"
}

so you do have a tag named child and you also have 2 tags of type child-tag, which to me seems consistent, a named tag always shows by name, and a tag always shows by tagname. Of course there would be overlap with 2 references to the same thing, but we seemed to have survived all these years with an html element having references like parentNode and parentElement which could be the same element.

[cognitom]

Hi. This is the latest version d3cbc92afbfa03c641fbaaeb5365ed3ed927a55b. Riot 3 will drop this[name-or-id-attr] and add this.refs[ref-attr]:

| property | Riot 2 | Riot 3 |
| :-- | :-- | :-- |
| 1 - this.root | DOM | DOM |
| 2 - this.parent | Tag | Tag |
| 3 - this.tags[custom-tag-name] | Tag / [Tag] | Tag / [Tag] |
| 4 - this.refs[ref-attr] | undefined | DOM / [DOM] / Tag / [Tag] |
| 5 - this.tags[name-or-id-attr] | Tag | undefined |
| 6 - this[name-or-id-attr] | DOM / [DOM] | undefined |

But this.refs seems still confusing. I think DOM / [DOM] is enough. Was this fully discussed already?

[GianlucaGuarini]

@cognitom you are totally right. Would you mind making another pull request please?

[rogueg]

I think if you put a ref on a tag, 90% of the time you want the tag instance, not the root DOM element.

[GianlucaGuarini]

@rogueg I am not sure, other frameworks like react and vue do not group instances and DOM nodes under the same key. Vue for example uses v-el:name for the DOM nodes and v-ref:name for the instances (even if it has some bugs). I would prefer to clean a bit the riot apis' with the newest release and having 4 different kind of types grouped under the same key seems to me really too much

[rogueg]

React works exactly the way I'm describing. From the docs:

When attaching a ref to a DOM component like <div />, you get the DOM node back; when attaching a ref to a composite component like <TextInput />, you'll get the React class instance. In the latter case, you can call methods on that component if any are exposed in its class definition.

I can see how the "sometimes an array, sometimes not" could be confusing. I don't have a strong opinion on how we handle that.

[GianlucaGuarini]

@rogueg I would like to hear also the @rsbondi opinion about this. I guess he will be back next week so let's keep this decision on hold

[cognitom]

Is it a possible option to drop tags and refs at all? We can handle them by querySelector like @aMarCruz said.
Or we could easily wrap querySelector like this:

<my-tag>
  <input class="first" value="a">
  <input class="first" value="b">
  <input-special class="second" value="x" />
  <p>{ $('.first').value }</p>
  <script>
    this.querySelector('.first').value // a
    this.querySelectorAll('.first')[1].value // b

    this.$('.first') // DOM
    this.$('.first', true)// undefined
    this.$('.second') // DOM
    this.$('.second', true) // Tag
  </script>
</my-tag>

• $: alias to querySelector
• $$: alias to querySelectorAll
• querySelector(selector, true): gets a Tag instance instead of DOM
• the references could be cached while update process
• we might need this conversion? $('input-special') --> $('input-special'),$([data-is='input-special'])

[cognitom]

I tried writing the code for proof of concept. #1934
I know it could be a dangerous change, but I'm starting to like it honestly...

[cognitom]

Sorry, I went on tangent a bit. The idea about querySelector has been landed on the independent mixin. No need to implement it into the core.

By the way, after week-long thought, I'd like to agree with @rogueg's comment. For the tag manipulation, we have native querySelect() API, so it seems a better idea to focus on Tag for me. And much consistency between this.tags and this.refs.

I'd like to wait the opinion from @rsbondi, too.

[rsbondi]

The way I see it,

• refs/tags gives overhead on tag creation, whether or not they are used, and adds complexity to the code base
• mixin gives overhead on tag creation but the user can choose if this functionality is needed or not, and use it globally or per tag
• The querySelector overhead on access is a concern as mentioned here but it seems best handled in the markup and not the javascript, at least in that example, and it seems more of an edge case

I would prefer not having the true flag for tag selection but this.$tag would seem nicer to me but not a deal breaker

So I am in line with @cognitom 's original train of thought, but I have been off the grid so I have not given it the "week long thought" so I may end up arriving at a different conclusion. And it would be quite a drastic change

[aMarCruz]

With the correction to this.refs having DOM / [DOM] only, the table from @cognitom seems good to me, or we can remove this.refs keeping a less confusing API. Anyway, riot3 is not riot2 anymore.

[rogueg]

What about getting rid of this.tags, and only having refs?

<my-tag ref='foo' />
<input ref='input' />

this.refs.foo   // => Tag(my-tag)
this.refs.input // => DOM

Maybe I've just been looking at it for too long, but this seems really straightforward to me.

[GianlucaGuarini]

I think at this point we have 2 plausible possibilities:

1. We could return always arrays for the refs and tags properties (my favorite)
2. We could store/manipulate/update the refs and tags properties as arrays internally and let them behave like the current riot using getter:

Object.defineProperty(this.tags, 'my-tag', {
  get: function() {
     var tags = this._internal.tags['my-tag']
     return tags.length > 1 ? tags : tags[0]
  }
})

[planeguy]

I like always arrays because you always know what's coming out of them. If you expect only one, then as a programmer you can just this.refs['myref'][0], otherwise just this.refs['myref'].forEach(doSomething)

It's probably slow, but I would just this.refs['myref'].forEach(doSomething) all the time

[juodumas]

I am also for always returning an array.

And I agree with @rogueg: this.refs[ref-attr] -> [DOM] / [Tag].

I need to access all child tags in a form helper mixin and I think there is no way to do it besides this.tags[custom-tag-name] -> [Tag], so I need this.tags to stay.

[cognitom]

@rogueg mixing [DOM] / [Tag] looks _not_ good to me. When we need to handle the DOM and Tag equally, we can wrap native <input> with <custom-input-or-something>. It keeps the code more consistent, I think:

<custom-input ref="name" value={ name } />
<date-picker ref="date" value={ date } />
this.refs['name'].on('change', item => {...})
this.refs['date'].on('change', item => {...})

---

@GianlucaGuarini "always arrays" is my favorite, but backward compatibility is important, too. Then, how about the separated methods? (your _getter_ idea seems also nice)

<my-tag ref="first" />
this.tags['my-tag'], this.tags['value-of-name-attr'] // Tag or [Tag] - same as Riot 2
this.getTagsByTagName('my-name') // [Tag] - always array
this.getTagsByRef('first') // [Tag] - always array

[planeguy]

^^^ heheheh that's what I get for going to a meeting before finishing a comment on github

side-thought: what's our standard way to tell if something is a tag vs. dom? Like if this.refs could return either, we should be able to tell right?

Personally I'd rather request what I want in the first place than request something in general and then check to see what it is. my rather verbose preference would be

| over-verbose property | returns |
| --- | --- |
| this.tagsNamed[tag-name] | [tag] |
| this.tagNamed[tag-name] | tag |
| this.domsByRef[ref-attr] | [dom] |
| this.domByRef[ref-attr] | dom |
| this.tagsByRef[ref-attr] | [tag] |
| this.tagByRef[ref-attr] | tag |

or something like that. like yeah it's more stuff, but you know what you're getting when you call it.

[ghost]

this.refs as an always array seems perfect to me as well.
Makes things predictable.
For backward compatibility, as is already said in this thread, a document on upgrading to riot3 mentioning this change should suffice.

[plumpNation]

I actually like the way it is. It's so simple and small, I don't really see the issue. You have very little to learn and remember already. Which is great.

this.root[.querySelector] is great! Please don't remove it.

[moritzschepp]

@GianlucaGuarini, as you posted earlier, I also find confusing that
<my-tag name='bar' /> => parent.tags['my-tag'] = undefined

Imagine something like
<my-field name="checkbox | text | radio" />

Here I would like to use the name attribute without that eliminating one way of access for the parent on its children. Specificically, It'd be convenient to
parent.tags['my-field'].forEach ...

Perhaps there could be a way to maintain access to all children of a specific type.

[GianlucaGuarini]

I think the we will clean up the ref and tags behavior in the next riot major release at moment any change to the riot api will be a breaking change. I am closing this issue appreciating the feedback received thank you