[FEATURE MODEL_ARG] Implement RFC emberjs/rfc#523 for {{mount}}

This is an extension to the RFC not explicitly written in the RFC text. I missed this when writing the RFC, but we felt that `{{mount}}` with the `model=` argument is even more clearly an argument, and that we explicitly decided to restrict the `{{mount}}` syntax to a single `model` argument (as opposed to arbitrary named arguments), so it is within the spirit of the RFC to make this work also. This also refactor the implementation of `{{mount}}` to do less custom work (like diffing the tag) and let Glimmer VM do more of the work via the usual paths.
emberjs · Sep 2, 2019 · 5b088ee · 5b088ee
1 parent d6fa505
commit 5b088ee
Show file tree

Hide file tree

Showing 4 changed files with 177 additions and 68 deletions.
diff --git a/packages/@ember/-internals/glimmer/lib/component-managers/mount.ts b/packages/@ember/-internals/glimmer/lib/component-managers/mount.ts
@@ -1,12 +1,15 @@
 import { DEBUG } from '@glimmer/env';
 import { ComponentCapabilities } from '@glimmer/interfaces';
-import { CONSTANT_TAG, Tag, validate, value, VersionedPathReference } from '@glimmer/reference';
-import { ComponentDefinition, Invocation, WithDynamicLayout } from '@glimmer/runtime';
+import { CONSTANT_TAG, Tag, VersionedPathReference } from '@glimmer/reference';
+import { Arguments, ComponentDefinition, Invocation, WithDynamicLayout } from '@glimmer/runtime';
 import { Destroyable, Opaque, Option } from '@glimmer/util';
 
 import { Owner } from '@ember/-internals/owner';
 import { generateControllerFactory } from '@ember/-internals/routing';
 import { OwnedTemplateMeta } from '@ember/-internals/views';
+import { EMBER_ROUTING_MODEL_ARG } from '@ember/canary-features';
+import { assert } from '@ember/debug';
+
 import { TemplateFactory } from '../..';
 import Environment from '../environment';
 import RuntimeResolver from '../resolver';
@@ -23,24 +26,18 @@ interface EngineState {
   engine: EngineInstance;
   controller: any;
   self: RootReference<any>;
-  tag: Tag;
-}
-
-interface EngineWithModelState extends EngineState {
-  modelRef: VersionedPathReference<Opaque>;
-  modelRev: number;
+  modelRef?: VersionedPathReference<Opaque>;
 }
 
 interface EngineDefinitionState {
   name: string;
-  modelRef: VersionedPathReference<Opaque> | undefined;
 }
 
 const CAPABILITIES = {
   dynamicLayout: true,
   dynamicTag: false,
   prepareArgs: false,
-  createArgs: false,
+  createArgs: true,
   attributeHook: false,
   elementHook: false,
   createCaller: true,
@@ -49,10 +46,17 @@ const CAPABILITIES = {
   createInstance: true,
 };
 
+// TODO
+// This "disables" the "@model" feature by making the arg untypable syntatically
+// Delete this when EMBER_ROUTING_MODEL_ARG has shipped
+export const MODEL_ARG_NAME = (EMBER_ROUTING_MODEL_ARG || !DEBUG)
+  ? 'model'
+  : ' untypable model arg ';
+
 class MountManager
-  extends AbstractManager<EngineState | EngineWithModelState, EngineDefinitionState>
+  extends AbstractManager<EngineState, EngineDefinitionState>
   implements
-    WithDynamicLayout<EngineState | EngineWithModelState, OwnedTemplateMeta, RuntimeResolver> {
+    WithDynamicLayout<EngineState, OwnedTemplateMeta, RuntimeResolver> {
   getDynamicLayout(state: EngineState, _: RuntimeResolver): Invocation {
     let templateFactory = state.engine.lookup('template:application') as TemplateFactory;
     let template = templateFactory(state.engine);
@@ -68,39 +72,40 @@ class MountManager
     return CAPABILITIES;
   }
 
-  create(environment: Environment, state: EngineDefinitionState) {
+  create(environment: Environment, { name }: EngineDefinitionState, args: Arguments) {
     if (DEBUG) {
-      this._pushEngineToDebugStack(`engine:${state.name}`, environment);
+      this._pushEngineToDebugStack(`engine:${name}`, environment);
     }
 
     // TODO
     // mount is a runtime helper, this shouldn't use dynamic layout
     // we should resolve the engine app template in the helper
     // it also should use the owner that looked up the mount helper.
 
-    let engine = environment.owner.buildChildEngineInstance<EngineInstance>(state.name);
+    let engine = environment.owner.buildChildEngineInstance<EngineInstance>(name);
 
     engine.boot();
 
     let applicationFactory = engine.factoryFor(`controller:application`);
     let controllerFactory = applicationFactory || generateControllerFactory(engine, 'application');
     let controller: any;
     let self: RootReference<any>;
-    let bucket: EngineState | EngineWithModelState;
-    let tag: Tag;
-    let modelRef = state.modelRef;
+    let bucket: EngineState;
+    let modelRef;
+
+    if (args.named.has(MODEL_ARG_NAME)) {
+      modelRef = args.named.get(MODEL_ARG_NAME);
+    }
+
     if (modelRef === undefined) {
       controller = controllerFactory.create();
       self = new RootReference(controller);
-      tag = CONSTANT_TAG;
-      bucket = { engine, controller, self, tag };
+      bucket = { engine, controller, self };
     } else {
       let model = modelRef.value();
-      let modelRev = value(modelRef.tag);
       controller = controllerFactory.create({ model });
       self = new RootReference(controller);
-      tag = modelRef.tag;
-      bucket = { engine, controller, self, tag, modelRef, modelRev };
+      bucket = { engine, controller, self, modelRef };
     }
 
     return bucket;
@@ -110,8 +115,12 @@ class MountManager
     return self;
   }
 
-  getTag(state: EngineState | EngineWithModelState): Tag {
-    return state.tag;
+  getTag(state: EngineState): Tag {
+    if (state.modelRef) {
+      return state.modelRef.tag;
+    } else {
+      return CONSTANT_TAG;
+    }
   }
 
   getDestructor({ engine }: EngineState): Option<Destroyable> {
@@ -124,13 +133,9 @@ class MountManager
     }
   }
 
-  update(bucket: EngineWithModelState): void {
-    let { controller, modelRef, modelRev } = bucket;
-    if (!validate(modelRef.tag, modelRev!)) {
-      let model = modelRef.value();
-      bucket.modelRev = value(modelRef.tag);
-      controller.set('model', model);
-    }
+  update({ controller, modelRef }: EngineState): void {
+    assert('[BUG] `update` should only be called when modelRef is present', modelRef !== undefined);
+    controller.set('model', modelRef!.value());
   }
 }
 
@@ -140,7 +145,7 @@ export class MountDefinition implements ComponentDefinition {
   public state: EngineDefinitionState;
   public manager = MOUNT_MANAGER;
 
-  constructor(name: string, modelRef: VersionedPathReference<Opaque> | undefined) {
-    this.state = { name, modelRef };
+  constructor(name: string) {
+    this.state = { name };
   }
 }
diff --git a/packages/@ember/-internals/glimmer/lib/syntax/mount.ts b/packages/@ember/-internals/glimmer/lib/syntax/mount.ts
@@ -3,18 +3,21 @@
 */
 import { OwnedTemplateMeta } from '@ember/-internals/views';
 import { assert } from '@ember/debug';
-import { Opaque, Option } from '@glimmer/interfaces';
+import { DEBUG } from '@glimmer/env';
+import { Option } from '@glimmer/interfaces';
 import { OpcodeBuilder } from '@glimmer/opcode-compiler';
 import { Tag, VersionedPathReference } from '@glimmer/reference';
 import {
   Arguments,
+  CapturedArguments,
   CurriedComponentDefinition,
   curry,
+  EMPTY_ARGS,
   UNDEFINED_REFERENCE,
   VM,
 } from '@glimmer/runtime';
 import * as WireFormat from '@glimmer/wire-format';
-import { MountDefinition } from '../component-managers/mount';
+import { MODEL_ARG_NAME, MountDefinition } from '../component-managers/mount';
 import Environment from '../environment';
 
 export function mountHelper(
@@ -23,8 +26,38 @@ export function mountHelper(
 ): VersionedPathReference<CurriedComponentDefinition | null> {
   let env = vm.env as Environment;
   let nameRef = args.positional.at(0);
-  let modelRef = args.named.has('model') ? args.named.get('model') : undefined;
-  return new DynamicEngineReference(nameRef, env, modelRef);
+  let captured: Option<CapturedArguments> = null;
+
+  // TODO: the functionailty to create a proper CapturedArgument should be
+  // exported by glimmer, or that it should provide an overload for `curry`
+  // that takes `PreparedArguments`
+  if (args.named.has('model')) {
+    assert('[BUG] this should already be checked by the macro', args.named.length === 1);
+
+    let named = args.named.capture();
+    let { tag } = named;
+
+    // TODO delete me after EMBER_ROUTING_MODEL_ARG has shipped
+    if (DEBUG && MODEL_ARG_NAME !== 'model') {
+      assert('[BUG] named._map is not null', named['_map'] === null);
+      named.names = [MODEL_ARG_NAME];
+    }
+
+    captured = {
+      tag,
+      positional: EMPTY_ARGS.positional,
+      named,
+      length: 1,
+      value() {
+        return {
+          named: this.named.value(),
+          positional: this.positional.value()
+        };
+      }
+    }
+  }
+
+  return new DynamicEngineReference(nameRef, env, captured);
 }
 
 /**
@@ -78,33 +111,38 @@ export function mountMacro(
     params!.length === 1
   );
 
+  if (DEBUG && hash) {
+    let keys = hash[0];
+    let extra = keys.filter(k => k !== 'model');
+
+    assert(
+      'You can only pass a `model` argument to the {{mount}} helper, ' +
+      'e.g. {{mount "profile-engine" model=this.profile}}. ' +
+      `You passed ${extra.join(',')}.`,
+      extra.length === 0
+    );
+  }
+
   let expr: WireFormat.Expressions.Helper = [WireFormat.Ops.Helper, '-mount', params || [], hash];
   builder.dynamicComponent(expr, null, [], null, false, null, null);
   return true;
 }
 
-class DynamicEngineReference {
+class DynamicEngineReference implements VersionedPathReference<Option<CurriedComponentDefinition>> {
   public tag: Tag;
-  public nameRef: VersionedPathReference<any | null | undefined>;
-  public modelRef: VersionedPathReference<Opaque> | undefined;
-  public env: Environment;
-  private _lastName: string | null;
-  private _lastDef: CurriedComponentDefinition | null;
+  private _lastName: Option<string> = null;
+  private _lastDef: Option<CurriedComponentDefinition> = null;
+
   constructor(
-    nameRef: VersionedPathReference<any | undefined | null>,
-    env: Environment,
-    modelRef: VersionedPathReference<Opaque> | undefined
+    public nameRef: VersionedPathReference<any | undefined | null>,
+    public env: Environment,
+    public args: Option<CapturedArguments>
   ) {
     this.tag = nameRef.tag;
-    this.nameRef = nameRef;
-    this.modelRef = modelRef;
-    this.env = env;
-    this._lastName = null;
-    this._lastDef = null;
   }
 
-  value() {
-    let { env, nameRef, modelRef } = this;
+  value(): Option<CurriedComponentDefinition> {
+    let { env, nameRef, args } = this;
     let name = nameRef.value();
 
     if (typeof name === 'string') {
@@ -122,7 +160,7 @@ class DynamicEngineReference {
       }
 
       this._lastName = name;
-      this._lastDef = curry(new MountDefinition(name, modelRef));
+      this._lastDef = curry(new MountDefinition(name), args);
 
       return this._lastDef;
     } else {

diff --git a/packages/@ember/-internals/glimmer/tests/integration/application/engine-test.js b/packages/@ember/-internals/glimmer/tests/integration/application/engine-test.js
@@ -1,10 +1,11 @@
 import { moduleFor, ApplicationTestCase, strip, runTaskNext } from 'internal-test-helpers';
 
-import Controller from '@ember/controller';
-import { RSVP } from '@ember/-internals/runtime';
 import { Component } from '@ember/-internals/glimmer';
-import Engine from '@ember/engine';
 import { Route } from '@ember/-internals/routing';
+import { RSVP } from '@ember/-internals/runtime';
+import { EMBER_ROUTING_MODEL_ARG } from '@ember/canary-features';
+import Controller from '@ember/controller';
+import Engine from '@ember/engine';
 import { next } from '@ember/runloop';
 
 import { compile } from '../../utils/helpers';
@@ -517,7 +518,12 @@ moduleFor(
             },
           })
         );
-        this.register('template:application_error', compile('Error! {{model.message}}'));
+        this.register(
+          'template:application_error',
+          compile(
+            EMBER_ROUTING_MODEL_ARG ? 'Error! {{@model.message}}' : 'Error! {{this.model.message}}'
+          )
+        );
         this.register(
           'route:post',
           Route.extend({
@@ -556,7 +562,12 @@ moduleFor(
             },
           })
         );
-        this.register('template:error', compile('Error! {{model.message}}'));
+        this.register(
+          'template:error',
+          compile(
+            EMBER_ROUTING_MODEL_ARG ? 'Error! {{@model.message}}' : 'Error! {{this.model.message}}'
+          )
+        );
         this.register(
           'route:post',
           Route.extend({
@@ -595,7 +606,12 @@ moduleFor(
             },
           })
         );
-        this.register('template:post_error', compile('Error! {{model.message}}'));
+        this.register(
+          'template:post_error',
+          compile(
+            EMBER_ROUTING_MODEL_ARG ? 'Error! {{@model.message}}' : 'Error! {{this.model.message}}'
+          )
+        );
         this.register(
           'route:post',
           Route.extend({
@@ -634,7 +650,12 @@ moduleFor(
             },
           })
         );
-        this.register('template:post.error', compile('Error! {{model.message}}'));
+        this.register(
+          'template:post.error',
+          compile(
+            EMBER_ROUTING_MODEL_ARG ? 'Error! {{@model.message}}' : 'Error! {{this.model.message}}'
+          )
+        );
         this.register(
           'route:post.comments',
           Route.extend({