From 8ad077a5385e3d2463be783a0e0a034ef03964b1 Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Mon, 2 Sep 2019 23:02:19 +0900 Subject: [PATCH] [FEATURE MODEL_ARG] Implement RFC emberjs/rfcs#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. --- .../glimmer/lib/component-managers/mount.ts | 73 ++++++++--------- .../-internals/glimmer/lib/syntax/mount.ts | 80 ++++++++++++++----- .../integration/application/engine-test.js | 35 ++++++-- .../glimmer/tests/integration/mount-test.js | 57 +++++++++++-- 4 files changed, 175 insertions(+), 70 deletions(-) diff --git a/packages/@ember/-internals/glimmer/lib/component-managers/mount.ts b/packages/@ember/-internals/glimmer/lib/component-managers/mount.ts index 35a35432b07..cd294e3148e 100644 --- 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; - tag: Tag; -} - -interface EngineWithModelState extends EngineState { - modelRef: VersionedPathReference; - modelRev: number; + modelRef?: VersionedPathReference; } interface EngineDefinitionState { name: string; - modelRef: VersionedPathReference | undefined; } const CAPABILITIES = { dynamicLayout: true, dynamicTag: false, prepareArgs: false, - createArgs: false, + createArgs: true, attributeHook: false, elementHook: false, createCaller: true, @@ -49,10 +46,13 @@ const CAPABILITIES = { createInstance: true, }; -class MountManager - extends AbstractManager - implements - WithDynamicLayout { +// 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 + implements WithDynamicLayout { getDynamicLayout(state: EngineState, _: RuntimeResolver): Invocation { let templateFactory = state.engine.lookup('template:application') as TemplateFactory; let template = templateFactory(state.engine); @@ -68,9 +68,9 @@ 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 @@ -78,7 +78,7 @@ class MountManager // 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(state.name); + let engine = environment.owner.buildChildEngineInstance(name); engine.boot(); @@ -86,21 +86,22 @@ class MountManager let controllerFactory = applicationFactory || generateControllerFactory(engine, 'application'); let controller: any; let self: RootReference; - 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 +111,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 { @@ -124,13 +129,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 +141,7 @@ export class MountDefinition implements ComponentDefinition { public state: EngineDefinitionState; public manager = MOUNT_MANAGER; - constructor(name: string, modelRef: VersionedPathReference | 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 index 71994fe39cd..0a6385ee21a 100644 --- 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 { 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 = 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> { public tag: Tag; - public nameRef: VersionedPathReference; - public modelRef: VersionedPathReference | undefined; - public env: Environment; - private _lastName: string | null; - private _lastDef: CurriedComponentDefinition | null; + private _lastName: Option = null; + private _lastDef: Option = null; + constructor( - nameRef: VersionedPathReference, - env: Environment, - modelRef: VersionedPathReference | undefined + public nameRef: VersionedPathReference, + public env: Environment, + public args: Option ) { 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 { + 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 index 953060821c8..297e955ca02 100644 --- 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({ diff --git a/packages/@ember/-internals/glimmer/tests/integration/mount-test.js b/packages/@ember/-internals/glimmer/tests/integration/mount-test.js index e8369f92db1..f76fe425763 100644 --- a/packages/@ember/-internals/glimmer/tests/integration/mount-test.js +++ b/packages/@ember/-internals/glimmer/tests/integration/mount-test.js @@ -1,11 +1,13 @@ import { moduleFor, ApplicationTestCase, RenderingTestCase, runTask } from 'internal-test-helpers'; +import { set } from '@ember/-internals/metal'; import { getOwner } from '@ember/-internals/owner'; -import { compile, Component } from '../utils/helpers'; +import { EMBER_ROUTING_MODEL_ARG } from '@ember/canary-features'; import Controller from '@ember/controller'; -import { set } from '@ember/-internals/metal'; import Engine, { getEngineParent } from '@ember/engine'; +import { compile, Component } from '../utils/helpers'; + moduleFor( '{{mount}} single param assertion', class extends RenderingTestCase { @@ -301,9 +303,14 @@ moduleFor( this._super(...arguments); this.register( 'template:application', - compile('

Param Engine: {{model.foo}}

', { - moduleName: 'my-app/templates/application.hbs', - }) + compile( + EMBER_ROUTING_MODEL_ARG + ? '

Param Engine: {{@model.foo}}

' + : '

Param Engine: {{this.model.foo}}

', + { + moduleName: 'my-app/templates/application.hbs', + } + ) ); }, }) @@ -390,7 +397,7 @@ moduleFor( this._super(...arguments); this.register( 'template:application', - compile('{{model.foo}}', { + compile(EMBER_ROUTING_MODEL_ARG ? '{{@model.foo}}' : '{{this.model.foo}}', { moduleName: 'my-app/templates/application.hbs', }) ); @@ -410,3 +417,41 @@ moduleFor( } } ); + +if (!EMBER_ROUTING_MODEL_ARG) { + moduleFor( + '{{mount}} params tests without @model', + class extends ApplicationTestCase { + constructor() { + super(...arguments); + + this.add( + 'engine:paramEngine', + Engine.extend({ + router: null, + init() { + this._super(...arguments); + this.register( + 'template:application', + compile('

@model: {{@model}}, this.model: {{this.model}}

', { + moduleName: 'my-app/templates/application.hbs', + }) + ); + }, + }) + ); + } + + ['@test it cannot access the model via @model']() { + this.router.map(function() { + this.route('engine-params'); + }); + this.addTemplate('engine-params', '{{mount "paramEngine" model="foo"}}'); + + return this.visit('/engine-params').then(() => { + this.assertInnerHTML('

@model: , this.model: foo

'); + }); + } + } + ); +}