From dec48d73bbe0d0189ae203eb12431ffc5521e0a2 Mon Sep 17 00:00:00 2001
From: Kevin Ngo <me@ngokevin.com>
Date: Thu, 9 Feb 2017 16:22:50 -0800
Subject: [PATCH] add warning if component declared after the scene in HTML
 (fixes #2307) (#2310)

---
 src/core/component.js            | 40 ++++++++++++++----
 src/core/scene/a-scene.js        |  9 +++-
 src/core/scene/scenes.js         |  4 ++
 src/index.js                     | 12 +++---
 tests/core/component.test.js     | 72 +++++++++++++++++++++++++++++++-
 tests/core/scene/a-scene.test.js | 34 ++++++++++++++-
 6 files changed, 156 insertions(+), 15 deletions(-)
 create mode 100644 src/core/scene/scenes.js

diff --git a/src/core/component.js b/src/core/component.js
index 658e92c5dd0..555da93ea34 100644
--- a/src/core/component.js
+++ b/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) {
diff --git a/src/core/scene/a-scene.js b/src/core/scene/a-scene.js
index 728747a66f4..bdb15b56fd6 100644
--- a/src/core/scene/a-scene.js
+++ b/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);
       }
     },
 
diff --git a/src/core/scene/scenes.js b/src/core/scene/scenes.js
new file mode 100644
index 00000000000..b6a9e3e1c6e
--- /dev/null
+++ b/src/core/scene/scenes.js
@@ -0,0 +1,4 @@
+/*
+  Scene index for keeping track of created scenes.
+*/
+module.exports = [];
diff --git a/src/index.js b/src/index.js
index 36d10d38b60..ba625880d8a 100644
--- a/src/index.js
+++ b/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,
diff --git a/tests/core/component.test.js b/tests/core/component.test.js
index ea739d349e1..b9320abb641 100644
--- a/tests/core/component.test.js
+++ b/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();
+    });
+  });
+});
diff --git a/tests/core/scene/a-scene.test.js b/tests/core/scene/a-scene.test.js
index 673569a8e96..f2d2399f0fb 100644
--- a/tests/core/scene/a-scene.test.js
+++ b/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);
+  });
+});