Merge pull request #18387 from emberjs/update-hook-fix

[BUGFIX] Ensure `updateComponent` is fired consistently
emberjs · Sep 14, 2019 · 865ffd0 · 865ffd0
2 parents 95e1e02 + f424b33
commit 865ffd0
Show file tree

Hide file tree

Showing 2 changed files with 87 additions and 36 deletions.
diff --git a/packages/@ember/-internals/glimmer/lib/component-managers/custom.ts b/packages/@ember/-internals/glimmer/lib/component-managers/custom.ts
@@ -12,7 +12,7 @@ import {
   Option,
   ProgramSymbolTable,
 } from '@glimmer/interfaces';
-import { PathReference, Tag } from '@glimmer/reference';
+import { createTag, isConst, PathReference, Tag } from '@glimmer/reference';
 import {
   Arguments,
   CapturedArguments,
@@ -324,7 +324,12 @@ export default class CustomComponentManager<ComponentInstance>
   }
 
   getTag({ args }: CustomComponentState<ComponentInstance>): Tag {
-    return args.tag;
+    if (isConst(args)) {
+      // returning a const tag skips the update hook (VM BUG?)
+      return createTag();
+    } else {
+      return args.tag;
+    }
   }
 
   didRenderLayout() {}

diff --git a/packages/@ember/-internals/glimmer/tests/integration/custom-component-manager-test.js b/packages/@ember/-internals/glimmer/tests/integration/custom-component-manager-test.js
@@ -1,4 +1,4 @@
-import { moduleFor, RenderingTestCase, runTask } from 'internal-test-helpers';
+import { moduleFor, RenderingTestCase, runTask, strip } from 'internal-test-helpers';
 
 import { Object as EmberObject } from '@ember/-internals/runtime';
 import { set, setProperties, computed } from '@ember/-internals/metal';
@@ -547,7 +547,7 @@ moduleFor(
       this.assertHTML(`<p>Chad Hietala</p>`);
     }
 
-    ['@test updating attributes triggers didUpdateComponent'](assert) {
+    ['@test updating attributes triggers updateComponent and didUpdateComponent'](assert) {
       let TestManager = EmberObject.extend({
         capabilities: capabilities('3.4', {
           destructor: true,
@@ -605,58 +605,104 @@ moduleFor(
       assert.verifySteps(['createComponent', 'getContext', 'didCreateComponent']);
 
       runTask(() => this.context.set('value', 'bar'));
-      assert.verifySteps(['didUpdateComponent']);
+      assert.verifySteps(['updateComponent', 'didUpdateComponent']);
     }
 
-    ['@test updating arguments does not trigger updateComponent or didUpdateComponent if `updateHook` is false'](
-      assert
-    ) {
-      let TestManager = EmberObject.extend({
-        capabilities: capabilities('3.13', {
-          updateHook: false,
-        }),
+    ['@test updateComponent fires consistently with or without args'](assert) {
+      let updated = [];
 
-        createComponent(factory, args) {
+      class TestManager {
+        static create() {
+          return new TestManager();
+        }
+
+        capabilities = capabilities('3.13', {
+          updateHook: true,
+        });
+
+        createComponent(_factory, args) {
           assert.step('createComponent');
-          return factory.create({ args });
-        },
+          return { id: args.named.id || 'no-id' };
+        }
 
-        updateComponent(component, args) {
+        updateComponent(component) {
           assert.step('updateComponent');
-          set(component, 'args', args);
-        },
-
-        destroyComponent(component) {
-          component.destroy();
-        },
+          updated.push(component);
+        }
 
         getContext(component) {
           assert.step('getContext');
           return component;
-        },
+        }
+      }
 
-        didUpdateComponent(component) {
-          assert.step('didUpdateComponent');
-          component.didUpdate();
-        },
+      let ComponentClass = setComponentManager(() => new TestManager(), {});
+
+      this.registerComponent('foo-bar', {
+        template: '{{yield}}',
+        ComponentClass,
       });
 
-      let ComponentClass = setComponentManager(
-        () => {
-          return TestManager.create();
-        },
-        EmberObject.extend({
-          didRender() {},
-          didUpdate() {},
-        })
+      this.render(
+        strip`
+          [<FooBar>{{this.value}}</FooBar>]
+          [<FooBar @id="static-id">{{this.value}}</FooBar>]
+          [<FooBar @id={{this.id}}>{{this.value}}</FooBar>]
+        `,
+        { id: 'dynamic-id', value: 'Hello World' }
       );
 
+      this.assertHTML(`[Hello World][Hello World][Hello World]`);
+      assert.deepEqual(updated, []);
+      assert.verifySteps([
+        'createComponent',
+        'getContext',
+        'createComponent',
+        'getContext',
+        'createComponent',
+        'getContext',
+      ]);
+
+      runTask(() => this.context.set('value', 'bar'));
+      assert.deepEqual(updated, [{ id: 'no-id' }, { id: 'static-id' }, { id: 'dynamic-id' }]);
+      assert.verifySteps(['updateComponent', 'updateComponent', 'updateComponent']);
+    }
+
+    ['@test updating arguments does not trigger updateComponent or didUpdateComponent if `updateHook` is false'](
+      assert
+    ) {
+      class TestManager {
+        capabilities = capabilities('3.13', {
+          /* implied: updateHook: false */
+        });
+
+        createComponent() {
+          assert.step('createComponent');
+          return {};
+        }
+
+        getContext(component) {
+          assert.step('getContext');
+          return component;
+        }
+
+        updateComponent() {
+          throw new Error('updateComponent called unexpectedly');
+        }
+
+        didUpdateComponent() {
+          throw new Error('didUpdateComponent called unexpectedly');
+        }
+      }
+
+      let ComponentClass = setComponentManager(() => new TestManager(), {});
+
       this.registerComponent('foo-bar', {
         template: `<p ...attributes>Hello world!</p>`,
         ComponentClass,
       });
 
-      this.render('<FooBar data-test={{value}} />', { value: 'foo' });
+      this.render('<FooBar data-test={{this.value}} />', { value: 'foo' });
 
       this.assertHTML(`<p data-test="foo">Hello world!</p>`);
       assert.verifySteps(['createComponent', 'getContext']);