add warning if component declared after the scene in HTML (fixes afra…

…mevr#2307) (aframevr#2310)

src/core/component.js

@@ -1,5 +1,6 @@
-/* global HTMLElement */
+/* global HTMLElement, Node */
 var schema = require('./schema');
+var scenes = require('./scene/scenes');
 var systems = require('./system');
 var utils = require('../utils/');
 
@@ -11,6 +12,10 @@ var isSingleProp = schema.isSingleProperty;
 var stringifyProperties = schema.stringifyProperties;
 var stringifyProperty = schema.stringifyProperty;
 var styleParser = utils.styleParser;
+var warn = utils.debug('core:component:warn');
+
+var aframeScript = document.currentScript;
+var upperCaseRegExp = new RegExp('[A-Z]+');
 
 /**
  * Component class definition.
@@ -261,6 +266,11 @@ Component.prototype = {
   }
 };
 
+// For testing.
+if (window.debug) {
+  var registrationOrderWarnings = module.exports.registrationOrderWarnings = {};
+}
+
 /**
  * Registers a component to A-Frame.
  *
@@ -272,13 +282,29 @@ module.exports.registerComponent = function (name, definition) {
   var NewComponent;
   var proto = {};
 
-  var testForUpperCase = new RegExp('[A-Z]+');
+  // Warning if component is statically registered after the scene.
+  if (document.currentScript && document.currentScript !== aframeScript) {
+    scenes.forEach(function checkPosition (sceneEl) {
+      // Okay to register component after the scene at runtime.
+      if (sceneEl.hasLoaded) { return; }
+
+      // Check that component is declared before the scene.
+      if (document.currentScript.compareDocumentPosition(sceneEl) ===
+          Node.DOCUMENT_POSITION_FOLLOWING) { return; }
+
+      warn('The component `' + name + '` was registered in a <script> tag after the scene. ' +
+           'Component <script> tags in an HTML file should be declared *before* the scene ' +
+           'such that the component is available to entities during scene initialization.');
+
+      // For testing.
+      if (window.debug) { registrationOrderWarnings[name] = true; }
+    });
+  }
 
-  if (testForUpperCase.test(name) === true) {
-    console.warn('The component name `' + name + '`contains uppercase characters,' +
-                  'but HTML ignores the capitalization of attributes. ' +
-                  'Consider changing it. ' +
-                  'Your component can be accessed as ' + name.toLowerCase());
+  if (upperCaseRegExp.test(name) === true) {
+    warn('The component name `' + name + '` contains uppercase characters, but ' +
+         'HTML will ignore the capitalization of attribute names. ' +
+         'Change the name to be lowercase: `' + name.toLowerCase() + '`');
   }
 
   if (name.indexOf('__') !== -1) {

src/core/scene/a-scene.js

@@ -2,6 +2,7 @@
 var initMetaTags = require('./metaTags').inject;
 var initWakelock = require('./wakelock');
 var re = require('../a-register-element');
+var scenes = require('./scenes');
 var systems = require('../system').systems;
 var THREE = require('../../lib/three');
 var TWEEN = require('tween.js');
@@ -56,7 +57,6 @@ module.exports = registerElement('a-scene', {
         this.render = bind(this.render, this);
         this.systems = {};
         this.time = 0;
-
         this.init();
       }
     },
@@ -105,6 +105,9 @@ module.exports = registerElement('a-scene', {
         window.addEventListener('load', resize);
         window.addEventListener('resize', resize);
         this.play();
+
+        // Add to scene index.
+        scenes.push(this);
       },
       writable: window.debug
     },
@@ -129,8 +132,12 @@ module.exports = registerElement('a-scene', {
      */
     detachedCallback: {
       value: function () {
+        var sceneIndex;
         window.cancelAnimationFrame(this.animationFrameID);
         this.animationFrameID = null;
+        // Remove from scene index.
+        sceneIndex = scenes.indexOf(this);
+        scenes.splice(sceneIndex, 1);
       }
     },
 

src/core/scene/scenes.js

@@ -0,0 +1,4 @@
+/*
+  Scene index for keeping track of created scenes.
+*/
+module.exports = [];

src/index.js

@@ -1,12 +1,13 @@
 var utils = require('./utils/');
 
 var debug = utils.debug;
-var error = debug('A-Frame:warn');
-var info = debug('A-Frame:info');
+var warn = debug('A-Frame:warn');
 
-if (document.currentScript && document.currentScript.parentNode !== document.head) {
-  error('Put the A-Frame <script> tag in the <head> of the HTML before <a-scene> to ensure everything for A-Frame is properly registered before they are used in the <body>.');
-  info('Also make sure that any component <script> tags are included after A-Frame, but still before <a-scene>.');
+if (document.currentScript && document.currentScript.parentNode !== document.head &&
+    !window.debug) {
+  warn('Put the A-Frame <script> tag in the <head> of the HTML *before* the scene to ' +
+       'ensure everything for A-Frame is properly registered before they are used from ' +
+       'HTML.');
 }
 
 // Polyfill `Promise`.
@@ -90,6 +91,7 @@ module.exports = window.AFRAME = {
     getMeshMixin: require('./extras/primitives/getMeshMixin'),
     primitives: require('./extras/primitives/primitives').primitives
   },
+  scenes: require('./core/scene/scenes'),
   schema: require('./core/schema'),
   shaders: shaders,
   systems: systems,

tests/core/component.test.js

@@ -1,6 +1,8 @@
-/* global AFRAME, assert, process, suite, test, setup, sinon, HTMLElement */
+/* global AFRAME, assert, process, suite, teardown, test, setup, sinon, HTMLElement */
 var buildData = require('core/component').buildData;
+var Component = require('core/component');
 var components = require('index').components;
+
 var helpers = require('../helpers');
 var registerComponent = require('index').registerComponent;
 var processSchema = require('core/schema').process;
@@ -669,3 +671,71 @@ suite('Component', function () {
     });
   });
 });
+
+suite('registerComponent warnings', function () {
+  var sceneEl;
+  var script;
+
+  setup(function (done) {
+    var el = entityFactory();
+    el.addEventListener('loaded', function () {
+      sceneEl = el.sceneEl;
+      script = document.createElement('script');
+      script.innerHTML = `AFRAME.registerComponent('testorder', {});`;
+      done();
+    });
+  });
+
+  teardown(function () {
+    delete AFRAME.components.testorder;
+    delete Component.registrationOrderWarnings.testorder;
+    if (script.parentNode) { script.parentNode.removeChild(script); }
+  });
+
+  test('does not throw warning if component registered in head', function (done) {
+    assert.notOk(Component.registrationOrderWarnings.testorder, 'waht');
+    document.head.appendChild(script);
+    setTimeout(() => {
+      assert.notOk(Component.registrationOrderWarnings.testorder);
+      done();
+    });
+  });
+
+  test('does not throw warning if component registered before scene', function (done) {
+    assert.notOk(Component.registrationOrderWarnings.testorder, 'foo');
+    document.body.insertBefore(script, sceneEl);
+    setTimeout(() => {
+      assert.notOk(Component.registrationOrderWarnings.testorder);
+      done();
+    });
+  });
+
+  test('does not throw warning if component registered after scene loaded', function (done) {
+    assert.notOk(Component.registrationOrderWarnings.testorder, 'blah');
+    sceneEl.addEventListener('loaded', () => {
+      document.body.appendChild(script);
+      setTimeout(() => {
+        assert.notOk(Component.registrationOrderWarnings.testorder);
+        done();
+      });
+    });
+  });
+
+  test('throws warning if component registered after scene', function (done) {
+    assert.notOk(Component.registrationOrderWarnings.testorder);
+    document.body.appendChild(script);
+    setTimeout(() => {
+      assert.ok(Component.registrationOrderWarnings.testorder);
+      done();
+    });
+  });
+
+  test('throws warning if component registered within scene', function (done) {
+    assert.notOk(Component.registrationOrderWarnings.testorder);
+    sceneEl.appendChild(script);
+    setTimeout(() => {
+      assert.ok(Component.registrationOrderWarnings.testorder);
+      done();
+    });
+  });
+});

tests/core/scene/a-scene.test.js

@@ -3,9 +3,11 @@ var AEntity = require('core/a-entity');
 var ANode = require('core/a-node');
 var AScene = require('core/scene/a-scene');
 var components = require('core/component').components;
-var helpers = require('../../helpers');
+var scenes = require('core/scene/scenes');
 var systems = require('core/system').systems;
 
+var helpers = require('../../helpers');
+
 /**
  * Tests in this suite should not involve WebGL contexts or renderer.
  * They operate with the assumption that attachedCallback is stubbed.
@@ -408,3 +410,33 @@ helpers.getSkipCISuite()('a-scene (with renderer)', function () {
     sinon.assert.calledWith(Component.tick, scene.time);
   });
 });
+
+suite('scenes', function () {
+  var sceneEl;
+
+  setup(function () {
+    scenes.length = 0;
+    sceneEl = document.createElement('a-scene');
+  });
+
+  test('is appended with scene attach', function (done) {
+    assert.notOk(scenes.length);
+    sceneEl.addEventListener('loaded', () => {
+      assert.ok(scenes.length);
+      done();
+    });
+    document.body.appendChild(sceneEl);
+  });
+
+  test('is popped with scene detached', function (done) {
+    sceneEl.addEventListener('loaded', () => {
+      assert.ok(scenes.length);
+      document.body.removeChild(sceneEl);
+      setTimeout(() => {
+        assert.notOk(scenes.length);
+        done();
+      });
+    });
+    document.body.appendChild(sceneEl);
+  });
+});