fix registerPrimitive property merging (fixes #1324)

Author: ngokevin

src/extras/primitives/registerPrimitive.js

@@ -4,7 +4,7 @@ var registerElement = require('../../core/a-register-element').registerElement;
 var utils = require('../../utils/');
 
 var debug = utils.debug;
-var log = debug('extras:primitives');
+var log = debug('extras:primitives:debug');
 
 module.exports = function registerPrimitive (name, definition) {
   name = name.toLowerCase();
@@ -46,16 +46,10 @@ module.exports = function registerPrimitive (name, definition) {
           var self = this;
           var attributes = this.attributes;
 
-          // Apply default components.
-          this.componentData = cloneObject(this.defaultAttributes);
-          Object.keys(this.componentData).forEach(function (componentName) {
-            if (!self.hasAttribute(componentName)) {
-              self.setAttribute(componentName, self.componentData[componentName]);
-            }
-          });
+          this.applyDefaultComponents();
 
           // Apply initial attributes.
-          Object.keys(attributes).forEach(function (attributeName) {
+          Object.keys(attributes).forEach(function applyInitial (attributeName) {
             var attr = attributes[attributeName];
             self.syncAttributeToComponent(attr.name, attr.value);
           });
@@ -75,6 +69,33 @@ module.exports = function registerPrimitive (name, definition) {
         }
       },
 
+      applyDefaultComponents: {
+        value: function () {
+          var self = this;
+          var defaultData = this.defaultAttributes;
+
+          // Apply default components.
+          Object.keys(defaultData).forEach(function applyDefault (componentName) {
+            var componentData = defaultData[componentName];
+
+            // Set component properties individually to not overwrite user-defined components.
+            if (componentData instanceof Object && Object.keys(componentData).length) {
+              Object.keys(componentData).forEach(function setProperty (propName) {
+                // Check if component property already defined.
+                var definedData = self.getAttribute(componentName);
+                if (definedData && definedData[propName] !== undefined) { return; }
+
+                self.setAttribute(componentName, propName, componentData[propName]);
+              });
+              return;
+            }
+
+            // Component is single-property schema, just set the attribute.
+            self.setAttribute(componentName, componentData);
+          });
+        }
+      },
+
       /**
        * If attribute is mapped to a component property, set the component property using
        * the attribute value.
@@ -104,20 +125,14 @@ module.exports = function registerPrimitive (name, definition) {
           // Run transform.
           value = this.getTransformedValue(attr, value);
 
-          // Initialize internal component data if necessary.
-          if (!this.componentData[componentName]) {
-            this.componentData[componentName] = this.defaultAttributes[componentName] || {};
-          }
-
-          // Update internal component data.
+          // If multi-property schema, set as update to component to not overwrite.
           if (propertyName) {
-            this.componentData[componentName][propertyName] = value;
-          } else {
-            this.componentData[componentName] = value;
+            this.setAttribute(componentName, propertyName, value);
+            return;
           }
 
-          // Put component data.
-          this.setAttribute(componentName, this.componentData[componentName]);
+          // Single-property schema, just set the value.
+          this.setAttribute(componentName, value);
         }
       },
 
@@ -133,21 +148,3 @@ module.exports = function registerPrimitive (name, definition) {
     })
   });
 };
-
-/**
- * Clone an object, including inner objects one-level deep.
- * Used for copying defaultAttributes to componentData so primitives of the same type don't
- * affect each others' defaultAttributes object.
- */
-function cloneObject (obj) {
-  var clone = {};
-  Object.keys(obj).forEach(function (key) {
-    var value = obj[key];
-    if (typeof value === 'object') {
-      clone[key] = utils.extend({}, value);
-    } else {
-      clone[key] = value;
-    }
-  });
-  return clone;
-}

tests/extras/primitives/registerPrimitive.test.js

@@ -4,12 +4,12 @@ var registerPrimitive = require('extras/primitives/registerPrimitive');
 
 var primitiveId = 0;
 
-function primitiveFactory (def, cb) {
+function primitiveFactory (definition, cb) {
   var el;
   var entity = helpers.entityFactory();
   var tagName = 'a-test-' + primitiveId++;
 
-  registerPrimitive(tagName, def);
+  registerPrimitive(tagName, definition);
   el = document.createElement(tagName);
   el.addEventListener('loaded', function () {
     cb(el, tagName);
@@ -18,20 +18,64 @@ function primitiveFactory (def, cb) {
 }
 
 suite('registerPrimitive', function () {
-  test('default attributes initialized', function (done) {
+  test('initializes default attributes', function (done) {
     primitiveFactory({
       defaultAttributes: {
-        material: { color: 'tomato' },
+        geometry: {primitive: 'box'},
+        material: {},
         position: '1 2 3'
       }
     }, function (el) {
-      assert.equal(el.getAttribute('material').color, 'tomato');
+      assert.equal(el.getAttribute('geometry').primitive, 'box');
+      assert.ok('material' in el.components);
       assert.equal(el.getAttribute('position').x, 1);
       done();
     });
   });
 
-  test('mapping proxy attributes to components', function (done) {
+  test('merges defined components with default components', function (done) {
+    var entity = helpers.entityFactory();
+    var tagName = 'a-test-' + primitiveId++;
+    registerPrimitive(tagName, {
+      defaultAttributes: {
+        material: {color: '#FFF', metalness: 0.63}
+      }
+    });
+
+    // Use innerHTML to set everything at once.
+    entity.innerHTML = '<' + tagName + ' material="color: tomato"></' + tagName + '>';
+    entity.addEventListener('loaded', function () {
+      var material = entity.children[0].getAttribute('material');
+      assert.equal(material.color, 'tomato');
+      assert.equal(material.metalness, 0.63);
+      done();
+    });
+  });
+
+  test('does not destroy defined components when proxying attributes', function (done) {
+    var entity = helpers.entityFactory();
+    var tag = 'a-test-' + primitiveId++;
+    registerPrimitive(tag, {
+      defaultAttributes: {
+        material: {color: '#FFF'}
+      },
+
+      mappings: {
+        color: 'material.color'
+      }
+    });
+
+    // Use innerHTML to set everything at once.
+    entity.innerHTML = '<' + tag + ' color="red" material="fog: false"></' + tag + '>';
+    entity.addEventListener('loaded', function () {
+      var material = entity.children[0].getAttribute('material');
+      assert.equal(material.color, 'red');
+      assert.equal(material.fog, 'false');
+      done();
+    });
+  });
+
+  test('proxies attributes to components', function (done) {
     primitiveFactory({
       mappings: {
         color: 'material.color',