[BUGFIX canary] Ensure elementId matches reality

Since the `id` attribute can be passed through splattributes, we cannot assume the generated `elementId` will be used. Instead of eagerly generating an `elementId` (if not set in `init`), we make it as lazy as possible and read from DOM after flushing the attributes. This ensures the value reflects what was eventually set. If, however, an `elementId` is explicitly set during `init`, we want to ensure they match up when `elementId` is accessed again. Note that with this commit, the framework no longer rely on `elementId` for any purpose. We may want to deprecate _reading_ from `elementId`, making it a "write-only" API that only serve to customize the wrapper element.
emberjs · Mar 29, 2019 · 85a34c0 · 85a34c0
1 parent c10a887
commit 85a34c0
Show file tree

Hide file tree

Showing 8 changed files with 115 additions and 70 deletions.
diff --git a/packages/@ember/-internals/glimmer/lib/component-managers/curly.ts b/packages/@ember/-internals/glimmer/lib/component-managers/curly.ts
@@ -1,9 +1,11 @@
 import { privatize as P } from '@ember/-internals/container';
 import { get } from '@ember/-internals/metal';
 import { getOwner } from '@ember/-internals/owner';
-import { guidFor } from '@ember/-internals/utils';
 import {
   addChildView,
+  getElementId,
+  getViewId,
+  hasElementId,
   OwnedTemplateMeta,
   setElementView,
   setViewElement,
@@ -92,7 +94,7 @@ function applyAttributeBindings(
   }
 
   if (seen.indexOf('id') === -1) {
-    let id = component.elementId ? component.elementId : guidFor(component);
+    let id = hasElementId(component) ? getElementId(component) : getViewId(component);
     operations.setAttribute('id', PrimitiveReference.create(id), false, null);
   }
 
@@ -352,7 +354,7 @@ export default class CurlyComponentManager
     if (attributeBindings && attributeBindings.length) {
       applyAttributeBindings(element, attributeBindings, component, operations);
     } else {
-      let id = component.elementId ? component.elementId : guidFor(component);
+      let id = hasElementId(component) ? getElementId(component) : getViewId(component);
       operations.setAttribute('id', PrimitiveReference.create(id), false, null);
       IsVisibleBinding.install(element, component, operations);
     }
@@ -521,17 +523,15 @@ export function processComponentInitializationAssertions(component: Component, p
   );
 
   assert(
-    `You cannot use \`classNameBindings\` on a tag-less component: ${component}`,
-    component.tagName !== '' ||
-      !component.classNameBindings ||
-      component.classNameBindings.length === 0
+    `You cannot use \`elementId\` on a tag-less component: ${component}`,
+    component.tagName !== '' || !hasElementId(component) || props.id === getElementId(component)
   );
 
   assert(
-    `You cannot use \`elementId\` on a tag-less component: ${component}`,
+    `You cannot use \`classNameBindings\` on a tag-less component: ${component}`,
     component.tagName !== '' ||
-      props.id === component.elementId ||
-      (!component.elementId && component.elementId !== '')
+      !component.classNameBindings ||
+      component.classNameBindings.length === 0
   );
 
   assert(

diff --git a/packages/@ember/-internals/glimmer/lib/utils/bindings.ts b/packages/@ember/-internals/glimmer/lib/utils/bindings.ts
@@ -1,4 +1,4 @@
-import { get } from '@ember/-internals/metal';
+import { getElementId, getViewId } from '@ember/-internals/views';
 import { assert } from '@ember/debug';
 import { dasherize } from '@ember/string';
 import { Opaque, Option, Simple } from '@glimmer/interfaces';
@@ -86,13 +86,8 @@ export const AttributeBinding = {
     let [prop, attribute, isSimple] = parsed;
 
     if (attribute === 'id') {
-      let elementId = get(component, prop);
-      if (elementId === undefined || elementId === null) {
-        elementId = component.elementId;
-      }
-      elementId = PrimitiveReference.create(elementId);
-      operations.setAttribute('id', elementId, true, null);
-      // operations.addStaticAttribute(element, 'id', elementId);
+      let elementId = component[prop] || getElementId(component) || getViewId(component);
+      operations.setAttribute('id', PrimitiveReference.create(elementId), true, null);
       return;
     }
 
@@ -111,7 +106,6 @@ export const AttributeBinding = {
     }
 
     operations.setAttribute(attribute, reference, false, null);
-    // operations.addDynamicAttribute(element, attribute, reference, false);
   },
 };
 

diff --git a/packages/@ember/-internals/glimmer/lib/utils/curly-component-state-bucket.ts b/packages/@ember/-internals/glimmer/lib/utils/curly-component-state-bucket.ts
@@ -1,7 +1,7 @@
 import { clearElementView, clearViewElement, getViewElement } from '@ember/-internals/views';
 import { Revision, VersionedReference } from '@glimmer/reference';
 import { CapturedNamedArguments } from '@glimmer/runtime';
-import { Opaque } from '@glimmer/util';
+import { Opaque, Option } from '@glimmer/util';
 import Environment from '../environment';
 
 export interface Component {
@@ -35,7 +35,7 @@ function NOOP() {}
   @private
 */
 export default class ComponentStateBucket {
-  public classRef: VersionedReference<Opaque> | null = null;
+  public classRef: Option<VersionedReference<Opaque>> = null;
   public argsRevision: Revision;
 
   constructor(

diff --git a/packages/@ember/-internals/glimmer/tests/integration/components/curly-components-test.js b/packages/@ember/-internals/glimmer/tests/integration/components/curly-components-test.js
@@ -99,7 +99,7 @@ moduleFor(
       if (EmberDev && !EmberDev.runningProdBuild) {
         let willThrow = () => run(null, set, component, 'elementId', 'herpyderpy');
 
-        assert.throws(willThrow, /Changing a view's elementId after creation is not allowed/);
+        assert.throws(willThrow, /cannot change `elementId` on <.+> once it is set/);
 
         this.assertComponentElement(this.firstChild, {
           tagName: 'div',
@@ -278,6 +278,23 @@ moduleFor(
       });
     }
 
+    ['@test elementId can not be a computed property']() {
+      let FooBarComponent = Component.extend({
+        elementId: computed(function() {
+          return 'foo-bar';
+        }),
+      });
+
+      this.registerComponent('foo-bar', {
+        ComponentClass: FooBarComponent,
+        template: 'hello',
+      });
+
+      expectAssertion(() => {
+        this.render('{{foo-bar}}');
+      }, /You cannot use a computed property for the component's `elementId` \(<.+?>\)\./);
+    }
+
     ['@test tagName can not be a computed property']() {
       let FooBarComponent = Component.extend({
         tagName: computed(function() {

diff --git a/packages/@ember/-internals/views/index.d.ts b/packages/@ember/-internals/views/index.d.ts
@@ -21,6 +21,9 @@ export const ViewMixin: any;
 export const ViewStateSupport: any;
 export const TextSupport: any;
 
+export function getElementId(view: Opaque): Option<string>;
+export function hasElementId(view: Opaque): boolean;
+
 export function registerView(view: Opaque): void;
 export function unregisterView(view: Opaque): void;
 

diff --git a/packages/@ember/-internals/views/index.js b/packages/@ember/-internals/views/index.js
@@ -25,7 +25,7 @@ export { default as CoreView } from './lib/views/core_view';
 export { default as ClassNamesSupport } from './lib/mixins/class_names_support';
 export { default as ChildViewsSupport } from './lib/mixins/child_views_support';
 export { default as ViewStateSupport } from './lib/mixins/view_state_support';
-export { default as ViewMixin } from './lib/mixins/view_support';
+export { default as ViewMixin, getElementId, hasElementId } from './lib/mixins/view_support';
 export { default as ActionSupport } from './lib/mixins/action_support';
 export { MUTABLE_CELL } from './lib/compat/attrs';
 export { default as lookupPartial, hasPartial } from './lib/system/lookup_partial';

diff --git a/packages/@ember/-internals/views/lib/mixins/view_support.js b/packages/@ember/-internals/views/lib/mixins/view_support.js
@@ -1,5 +1,6 @@
-import { guidFor } from '@ember/-internals/utils';
 import { descriptorForProperty, Mixin, nativeDescDecorator } from '@ember/-internals/metal';
+import { symbol } from '@ember/-internals/utils';
+import { getViewElement } from '@ember/-internals/views';
 import { assert } from '@ember/debug';
 import { hasDOM } from '@ember/-internals/browser-environment';
 import { matches } from '../system/utils';
@@ -11,6 +12,17 @@ function K() {
   return this;
 }
 
+const ELEMENT_ID = symbol('elementId');
+
+export function hasElementId(view) {
+  let elementId = view[ELEMENT_ID];
+  return elementId !== null && elementId !== undefined;
+}
+
+export function getElementId(view) {
+  return view[ELEMENT_ID];
+}
+
 let mixin = {
   /**
    A list of properties of the view to apply as attributes. If the property
@@ -154,6 +166,71 @@ let mixin = {
     },
   }),
 
+  /**
+   The HTML `id` of the view's element in the DOM. You can provide this
+   value yourself but it must be unique (just as in HTML):
+
+   ```handlebars
+   {{my-component elementId="a-really-cool-id"}}
+   ```
+
+   If not manually set a default value will be provided by the framework.
+
+   Once rendered an element's `elementId` is considered immutable and you
+   should never change it. If you need to compute a dynamic value for the
+   `elementId`, you should do this when the component or element is being
+   instantiated:
+
+   ```app/components/my-component.js
+   import Component from '@ember/component';
+
+   export default Component.extend({
+      init() {
+        this._super(...arguments);
+        let index = this.get('index');
+        this.set('elementId', 'component-id' + index);
+      }
+    });
+   ```
+
+   @property elementId
+   @type String
+   @public
+   */
+  elementId: nativeDescDecorator({
+    configurable: false,
+    enumerable: false,
+    get() {
+      let value = this[ELEMENT_ID];
+
+      assert(
+        `\`elementId\` on ${this} does not match the \`id\` attribute in DOM`,
+        !(value && getViewElement(this) && getViewElement(this).getAttribute('id') !== value)
+      );
+
+      if (!value) {
+        let element = getViewElement(this);
+        let id = element && element.getAttribute('id');
+
+        if (id) {
+          value = this[ELEMENT_ID] = element.getAttribute('id');
+        }
+      }
+
+      return value;
+    },
+    set(value) {
+      assert(
+        `cannot change \`elementId\` on ${this} once it is set`,
+        this[ELEMENT_ID] === null || this[ELEMENT_ID] === undefined
+      );
+
+      assert(`cannot set \`elementId\` on ${this} after rendering`, !getViewElement(this));
+
+      this[ELEMENT_ID] = value ? String(value) : null;
+    },
+  }),
+
   /**
    Appends the view's element to the specified parent element.
 
@@ -233,39 +310,6 @@ let mixin = {
     return this.appendTo(document.body);
   },
 
-  /**
-   The HTML `id` of the view's element in the DOM. You can provide this
-   value yourself but it must be unique (just as in HTML):
-
-   ```handlebars
-   {{my-component elementId="a-really-cool-id"}}
-   ```
-
-   If not manually set a default value will be provided by the framework.
-
-   Once rendered an element's `elementId` is considered immutable and you
-   should never change it. If you need to compute a dynamic value for the
-   `elementId`, you should do this when the component or element is being
-   instantiated:
-
-   ```app/components/my-component.js
-   import Component from '@ember/component';
-
-   export default Component.extend({
-      init() {
-        this._super(...arguments);
-        let index = this.get('index');
-        this.set('elementId', 'component-id' + index);
-      }
-    });
-   ```
-
-   @property elementId
-   @type String
-   @public
-   */
-  elementId: null,
-
   /**
    Called when a view is going to insert an element into the DOM.
 
@@ -396,10 +440,6 @@ let mixin = {
       descriptorForProperty(this, 'tagName') === undefined
     );
 
-    if (!this.elementId && this.tagName !== '') {
-      this.elementId = guidFor(this);
-    }
-
     assert('Using a custom `.render` function is no longer supported.', !this.render);
   },
 

diff --git a/packages/@ember/-internals/views/lib/views/states/in_dom.js b/packages/@ember/-internals/views/lib/views/states/in_dom.js
@@ -1,20 +1,11 @@
 import { assign } from '@ember/polyfills';
-import { addObserver } from '@ember/-internals/metal';
-import EmberError from '@ember/error';
-import { DEBUG } from '@glimmer/env';
 import hasElement from './has_element';
 
 const inDOM = assign({}, hasElement, {
   enter(view) {
     // Register the view for event handling. This hash is used by
     // Ember.EventDispatcher to dispatch incoming events.
     view.renderer.register(view);
-
-    if (DEBUG) {
-      addObserver(view, 'elementId', () => {
-        throw new EmberError("Changing a view's elementId after creation is not allowed");
-      });
-    }
   },
 
   exit(view) {