Multiple components

[dmarcos]

Description:

It allows defining multiple component of a specific kind by appending an id to the component name followed by a __ character. e.g:

el.setAttribute('sound__1', { src: '...' });
el.setAttribute('sound__explosion', { src: '...' });

Components have to opt into multiplicity otherwise an error is thrown.

This PR depends on #1565

Needs feedback from @fernandojsg

[ngokevin]

The overall API looks good to me. We might consider a double underscore to further make it look different from normal component name hyphens (like OOCSS namespaces).

[fernandojsg]

I agree with @ngokevin having double underscore maybe looks uglier but it will make it easier to differentiate between normal components and multiple instances

[fernandojsg]

I'm trying this PR on the editor. Please share what you think about the way to show this feature: aframevr/aframe-inspector#72 /cc @ngokevin @dmarcos

[fernandojsg]

I've been playing with your example examples/test/cube:
Querying each component return the expected results:

But when I try to change one instance's attribute something weird happens: it generates a new sound component, copying the same values from sound_2 but the volume is being copied as string instead of number.

[fernandojsg]

It fails just on debug=true as in the flushToDOM you're using just the this.name for the component (sound) instead of the whole component_id name:
This:

  flushToDOM: function () {
    var attrValue = this.attrValue;
    if (!attrValue) { return; }
    HTMLElement.prototype.setAttribute.call(this.el, this.name, this.stringify(attrValue));
  },

Should be something like:

  flushToDOM: function () {
    var attrValue = this.attrValue;
    if (!attrValue) { return; }
    var nameId = this.name + (this.id ? '_' + this.id : '');
    HTMLElement.prototype.setAttribute.call(this.el, nameId, this.stringify(attrValue));
  },

I'm thinking also about componentChanged event:

    el.emit('componentchanged', {
      name: this.name,
      newData: this.getData(),
      oldData: oldData
    });

With the current implementation it will emit just sound name event without any additional information, we should add the full name sound_{id} or at least an additional id parameter

[fernandojsg]

why don't we use here the same style as the component's name? sound__

[ngokevin]

Won't this error? 'geometry'.split('__')[1]

[ngokevin]

Name componentId might help differentiate from just HTML ID.

[fernandojsg]

it will return undefined, we could leave it that way or do something like componentInfo[1] || null. In any case it should works as expected on the rest of the code where the full name is built using: componentName = id ? component + '__' + id : component and both undefined or null will return false.

[dmarcos]

No it just returns undefined

[fernandojsg]

umm, maybe instanceId ? and the componentId could be the whole name componentName__instanceId?

[dmarcos]

I think the id has to be still easily accessible. Each component is going to handle multiplicity in a different way and it's nice to have access to the id directly without having to parse any string. I can add this.name, this.attrName, this.id to the component.

[ngokevin]

Would update to The sequence__(double underscore) is reserved for multiple components of the same type. since the "cannot be used" is sort of restating the first sentence