Don't cache parsed value of selector(All) property types (#5521)

Co-authored-by: Noeri Huisman <mrxz@users.noreply.github.com>

Author: mrxz

src/core/component.js

@@ -426,7 +426,8 @@ Component.prototype = {
 
       // Parse the new value into attrValue (re-using objects where possible)
       var newAttrValue = key ? this.attrValue[key] : this.attrValue;
-      newAttrValue = parseProperty(newValue, propertySchema, newAttrValue);
+      // Some property types (like selectors) depend on external state (e.g. DOM) during parsing and can't be cached.
+      newAttrValue = propertySchema.isCacheable ? parseProperty(newValue, propertySchema, newAttrValue) : newValue;
       // In case the output is a string, store the unparsed value (no double parsing and helps inspector)
       if (typeof newAttrValue === 'string') {
         // Quirk: empty strings aren't considered values for single-property schemas

src/core/propertyTypes.js

@@ -1,7 +1,6 @@
 var coordinates = require('../utils/coordinates');
 var debug = require('debug');
 
-var error = debug('core:propertyTypes:warn');
 var warn = debug('core:propertyTypes:warn');
 
 var propertyTypes = module.exports.propertyTypes = {};
@@ -13,15 +12,15 @@ registerPropertyType('audio', '', assetParse);
 registerPropertyType('array', [], arrayParse, arrayStringify, arrayEquals);
 registerPropertyType('asset', '', assetParse);
 registerPropertyType('boolean', false, boolParse);
-registerPropertyType('color', '#FFF', defaultParse, defaultStringify);
+registerPropertyType('color', '#FFF');
 registerPropertyType('int', 0, intParse);
 registerPropertyType('number', 0, numberParse);
 registerPropertyType('map', '', assetParse);
 registerPropertyType('model', '', assetParse);
-registerPropertyType('selector', null, selectorParse, selectorStringify);
-registerPropertyType('selectorAll', null, selectorAllParse, selectorAllStringify);
+registerPropertyType('selector', null, selectorParse, selectorStringify, defaultEquals, false);
+registerPropertyType('selectorAll', null, selectorAllParse, selectorAllStringify, arrayEquals, false);
 registerPropertyType('src', '', srcParse);
-registerPropertyType('string', '', defaultParse, defaultStringify);
+registerPropertyType('string', '');
 registerPropertyType('time', 0, intParse);
 registerPropertyType('vec2', {x: 0, y: 0}, vecParse, coordinates.stringify, coordinates.equals);
 registerPropertyType('vec3', {x: 0, y: 0, z: 0}, vecParse, coordinates.stringify, coordinates.equals);
@@ -37,8 +36,9 @@ registerPropertyType('vec4', {x: 0, y: 0, z: 0, w: 1}, vecParse, coordinates.str
  * @param {function} [parse=defaultParse] - Parse string function.
  * @param {function} [stringify=defaultStringify] - Stringify to DOM function.
  * @param {function} [equals=defaultEquals] - Equality comparator.
+ * @param {boolean} [cachable=false] - Whether or not the parsed value of a property can be cached.
  */
-function registerPropertyType (type, defaultValue, parse, stringify, equals) {
+function registerPropertyType (type, defaultValue, parse, stringify, equals, cacheable) {
   if (type in propertyTypes) {
     throw new Error('Property type ' + type + ' is already registered.');
   }
@@ -47,7 +47,8 @@ function registerPropertyType (type, defaultValue, parse, stringify, equals) {
     default: defaultValue,
     parse: parse || defaultParse,
     stringify: stringify || defaultStringify,
-    equals: equals || defaultEquals
+    equals: equals || defaultEquals,
+    isCacheable: cacheable !== false
   };
 }
 module.exports.registerPropertyType = registerPropertyType;

src/core/schema.js

@@ -83,6 +83,7 @@ function processPropertyDefinition (propDefinition, componentName) {
   propDefinition.parse = propDefinition.parse || propType.parse;
   propDefinition.stringify = propDefinition.stringify || propType.stringify;
   propDefinition.equals = propDefinition.equals || propType.equals;
+  propDefinition.isCacheable = propDefinition.isCacheable === true || propType.isCacheable;
 
   // Fill in type name.
   propDefinition.type = typeName;

tests/core/a-entity.test.js

@@ -455,6 +455,33 @@ suite('a-entity', function () {
       assert.notOk(geometry.width);
     });
 
+    test('do not cache properties with non-cacheable types', function () {
+      el.setAttribute('light', { target: '#some-element' });
+      assert.deepEqual(el.components.light.attrValue, {target: '#some-element'});
+
+      const light = el.getAttribute('light');
+      assert.notOk(light.target);
+    });
+
+    test('non-cacheable selectors are parsed at component initialization', function (done) {
+      const newEl = document.createElement('a-entity');
+      newEl.setAttribute('light', { target: '#target-element' });
+
+      // Light refers to target that doesn't exist yet.
+      assert.deepEqual(newEl.components.light.attrValue, {target: '#target-element'});
+      assert.notOk(newEl.getAttribute('light').target);
+
+      // Setup target element.
+      el.id = 'target-element';
+      el.appendChild(newEl);
+
+      newEl.addEventListener('loaded', function () {
+        assert.deepEqual(newEl.components.light.attrValue, {target: '#target-element'});
+        assert.strictEqual(newEl.getAttribute('light').target, el);
+        done();
+      });
+    });
+
     test('parses individual properties when passing object', function (done) {
       AFRAME.registerComponent('foo', {
         schema: {