From 1a1686592e9c32b0c9b64e344e03283f23d0a225 Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Fri, 22 Nov 2024 10:43:11 -0800 Subject: [PATCH] [FEATURE] Allow components in routes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second attempt at #20768 * Refactor to make component the primary path, normalizes templates into a custom component with `{{this}}` set to controller * Keep the core `{{outlet}}` responsibilities separate from what goes into the outlet * Ensures proper debug render tree output – `route-template` node only appears when a template (the custom component) is used * Ensures this works with test-helpers & others that * It doesn't differentiate between components and templates once normalized so `@controller` and `@model` is passed to both TODO: * Feature flag * Test and integrate this in @ember/test-helpers + smoke test * Inlined some TODO comments to investigate/simplify things Co-authored-by: Edward Faulkner --- .eslintrc.js | 6 + .../glimmer/lib/component-managers/outlet.ts | 109 ++++++------ .../lib/component-managers/route-template.ts | 162 ++++++++++++++++++ .../@ember/-internals/glimmer/lib/renderer.ts | 36 +++- .../-internals/glimmer/lib/syntax/outlet.ts | 110 ++++++++++-- .../-internals/glimmer/lib/utils/outlet.ts | 5 +- .../-internals/glimmer/lib/views/outlet.ts | 1 - .../application/debug-render-tree-test.ts | 91 ++++++++-- .../integration/application/rendering-test.js | 93 +++++++++- .../glimmer/tests/integration/mount-test.js | 10 +- .../glimmer/tests/utils/debug-stack.js | 8 +- packages/@ember/routing/route.ts | 59 ++++++- pnpm-lock.yaml | 128 ++++++++++++-- .../app/controllers/example-gjs-route.js | 5 + smoke-tests/app-template/app/router.js | 4 +- .../app/routes/example-gjs-route.js | 8 + .../app/templates/application.hbs | 5 +- .../app/templates/example-gjs-route.gjs | 13 ++ .../app-template/app/templates/index.hbs | 4 + smoke-tests/app-template/package.json | 4 +- .../acceptance/example-gjs-route-test.js | 15 ++ 21 files changed, 751 insertions(+), 125 deletions(-) create mode 100644 packages/@ember/-internals/glimmer/lib/component-managers/route-template.ts create mode 100644 smoke-tests/app-template/app/controllers/example-gjs-route.js create mode 100644 smoke-tests/app-template/app/routes/example-gjs-route.js create mode 100644 smoke-tests/app-template/app/templates/example-gjs-route.gjs create mode 100644 smoke-tests/app-template/app/templates/index.hbs create mode 100644 smoke-tests/app-template/tests/acceptance/example-gjs-route-test.js diff --git a/.eslintrc.js b/.eslintrc.js index eb665f5eaaf..bbf27006b25 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -26,6 +26,12 @@ module.exports = { 'disable-features/disable-async-await': 'error', 'disable-features/disable-generator-functions': 'error', + 'import/no-unresolved': [ + 'error', + { + ignore: ['@ember/template-compiler'], + }, + ], }, settings: { diff --git a/packages/@ember/-internals/glimmer/lib/component-managers/outlet.ts b/packages/@ember/-internals/glimmer/lib/component-managers/outlet.ts index 4503ffcb43d..449576f4483 100644 --- a/packages/@ember/-internals/glimmer/lib/component-managers/outlet.ts +++ b/packages/@ember/-internals/glimmer/lib/component-managers/outlet.ts @@ -1,25 +1,23 @@ import type { InternalOwner } from '@ember/-internals/owner'; +import type { Nullable } from '@ember/-internals/utility-types'; import { assert } from '@ember/debug'; import EngineInstance from '@ember/engine/instance'; import { _instrumentStart } from '@ember/instrumentation'; +import { precompileTemplate } from '@ember/template-compilation'; import type { - CapturedArguments, CompilableProgram, ComponentDefinition, - CapabilityMask, CustomRenderNode, Destroyable, Environment, InternalComponentCapabilities, - Template, VMArguments, WithCreateInstance, WithCustomDebugRenderTree, } from '@glimmer/interfaces'; -import type { Nullable } from '@ember/-internals/utility-types'; import { capabilityFlagsFrom } from '@glimmer/manager'; import type { Reference } from '@glimmer/reference'; -import { createConstRef, valueForRef } from '@glimmer/reference'; +import { UNDEFINED_REFERENCE, valueForRef } from '@glimmer/reference'; import { EMPTY_ARGS } from '@glimmer/runtime'; import { unwrapTemplate } from '@glimmer/util'; @@ -33,19 +31,18 @@ function instrumentationPayload(def: OutletDefinitionState) { } interface OutletInstanceState { - self: Reference; - outletBucket?: {}; - engineBucket?: { mountPoint: string }; - engine?: EngineInstance; + engine?: { + instance: EngineInstance; + mountPoint: string; + }; finalize: () => void; } export interface OutletDefinitionState { ref: Reference; name: string; - template: Template; + template: object; controller: unknown; - model: unknown; } const CAPABILITIES: InternalComponentCapabilities = { @@ -64,9 +61,11 @@ const CAPABILITIES: InternalComponentCapabilities = { hasSubOwner: false, }; +const CAPABILITIES_MASK = capabilityFlagsFrom(CAPABILITIES); + class OutletComponentManager implements - WithCreateInstance, + WithCreateInstance, WithCustomDebugRenderTree { create( @@ -79,19 +78,21 @@ class OutletComponentManager let parentStateRef = dynamicScope.get('outletState'); let currentStateRef = definition.ref; + // This is the actual primary responsibility of the outlet component – + // it represents the switching from one route component/template into + // the next. The rest only exists to support the debug render tree and + // the old-school (and unreliable) instrumentation. dynamicScope.set('outletState', currentStateRef); let state: OutletInstanceState = { - self: createConstRef(definition.controller, 'this'), finalize: _instrumentStart('render.outlet', instrumentationPayload, definition), }; if (env.debugRenderTree !== undefined) { - state.outletBucket = {}; - let parentState = valueForRef(parentStateRef); - let parentOwner = parentState && parentState.render && parentState.render.owner; - let currentOwner = valueForRef(currentStateRef)!.render!.owner; + let parentOwner = parentState?.render?.owner; + let currentState = valueForRef(currentStateRef); + let currentOwner = currentState?.render?.owner; if (parentOwner && parentOwner !== currentOwner) { assert( @@ -99,12 +100,13 @@ class OutletComponentManager currentOwner instanceof EngineInstance ); - let mountPoint = currentOwner.mountPoint; - - state.engine = currentOwner; + let { mountPoint } = currentOwner; if (mountPoint) { - state.engineBucket = { mountPoint }; + state.engine = { + mountPoint, + instance: currentOwner, + }; } } } @@ -112,21 +114,18 @@ class OutletComponentManager return state; } - getDebugName({ name }: OutletDefinitionState) { - return name; + getDebugName({ name }: OutletDefinitionState): string { + return `{{outlet}} for ${name}`; } getDebugCustomRenderTree( - definition: OutletDefinitionState, - state: OutletInstanceState, - args: CapturedArguments + _definition: OutletDefinitionState, + state: OutletInstanceState ): CustomRenderNode[] { let nodes: CustomRenderNode[] = []; - assert('[BUG] outletBucket must be set', state.outletBucket); - nodes.push({ - bucket: state.outletBucket, + bucket: state, type: 'outlet', // "main" used to be the outlet name, keeping it around for compatibility name: 'main', @@ -135,26 +134,17 @@ class OutletComponentManager template: undefined, }); - if (state.engineBucket) { + if (state.engine) { nodes.push({ - bucket: state.engineBucket, + bucket: state.engine, type: 'engine', - name: state.engineBucket.mountPoint, + name: state.engine.mountPoint, args: EMPTY_ARGS, - instance: state.engine, + instance: state.engine.instance, template: undefined, }); } - nodes.push({ - bucket: state, - type: 'route-template', - name: definition.name, - args: args, - instance: definition.controller, - template: unwrapTemplate(definition.template).moduleName, - }); - return nodes; } @@ -162,8 +152,8 @@ class OutletComponentManager return CAPABILITIES; } - getSelf({ self }: OutletInstanceState) { - return self; + getSelf() { + return UNDEFINED_REFERENCE; } didCreate() {} @@ -182,30 +172,27 @@ class OutletComponentManager const OUTLET_MANAGER = new OutletComponentManager(); -export class OutletComponentDefinition +const OUTLET_COMPONENT_TEMPLATE = precompileTemplate( + '<@Component @controller={{@controller}} @model={{@model}} />', + { strictMode: true } +); + +export class OutletComponent implements ComponentDefinition { // handle is not used by this custom definition public handle = -1; - - public resolvedName: string; + public resolvedName = null; + public manager = OUTLET_MANAGER; + public capabilities = CAPABILITIES_MASK; public compilable: CompilableProgram; - public capabilities: CapabilityMask; - - constructor( - public state: OutletDefinitionState, - public manager: OutletComponentManager = OUTLET_MANAGER - ) { - let capabilities = manager.getCapabilities(); - this.capabilities = capabilityFlagsFrom(capabilities); - this.compilable = capabilities.wrapped - ? unwrapTemplate(state.template).asWrappedLayout() - : unwrapTemplate(state.template).asLayout(); - this.resolvedName = state.name; + + constructor(owner: InternalOwner, public state: OutletDefinitionState) { + this.compilable = unwrapTemplate(OUTLET_COMPONENT_TEMPLATE(owner)).asLayout(); } } -export function createRootOutlet(outletView: OutletView): OutletComponentDefinition { - return new OutletComponentDefinition(outletView.state); +export function createRootOutlet(outletView: OutletView): OutletComponent { + return new OutletComponent(outletView.owner, outletView.state); } diff --git a/packages/@ember/-internals/glimmer/lib/component-managers/route-template.ts b/packages/@ember/-internals/glimmer/lib/component-managers/route-template.ts new file mode 100644 index 00000000000..4277f89323d --- /dev/null +++ b/packages/@ember/-internals/glimmer/lib/component-managers/route-template.ts @@ -0,0 +1,162 @@ +import type { InternalOwner } from '@ember/-internals/owner'; +import { _instrumentStart } from '@ember/instrumentation'; +import type { + CapturedArguments, + CompilableProgram, + ComponentDefinition, + CustomRenderNode, + Destroyable, + InternalComponentCapabilities, + Template, + VMArguments, + WithCreateInstance, + WithCustomDebugRenderTree, +} from '@glimmer/interfaces'; +import type { Nullable } from '@ember/-internals/utility-types'; +import { DEBUG } from '@glimmer/env'; +import { capabilityFlagsFrom } from '@glimmer/manager'; +import type { Reference } from '@glimmer/reference'; +import { createDebugAliasRef, valueForRef } from '@glimmer/reference'; +import { curry, type CurriedValue } from '@glimmer/runtime'; +import { unwrapTemplate } from '@glimmer/util'; +import { CurriedType } from '@glimmer/vm'; + +interface RouteTemplateInstanceState { + self: Reference; + controller: unknown; +} + +export interface RouteTemplateDefinitionState { + name: string; + templateName: string; +} + +const CAPABILITIES: InternalComponentCapabilities = { + dynamicLayout: false, + dynamicTag: false, + prepareArgs: false, + createArgs: true, + attributeHook: false, + elementHook: false, + createCaller: false, + dynamicScope: false, + updateHook: false, + createInstance: true, + wrapped: false, + willDestroy: false, + hasSubOwner: false, +}; + +const CAPABILITIES_MASK = capabilityFlagsFrom(CAPABILITIES); + +class RouteTemplateManager + implements + WithCreateInstance, + WithCustomDebugRenderTree +{ + create( + _owner: InternalOwner, + _definition: RouteTemplateDefinitionState, + args: VMArguments + ): RouteTemplateInstanceState { + let self = args.named.get('controller'); + + if (DEBUG) { + self = createDebugAliasRef!('this', self); + } + + let controller = valueForRef(self); + + return { self, controller }; + } + + getSelf({ self }: RouteTemplateInstanceState): Reference { + return self; + } + + getDebugName({ name }: RouteTemplateDefinitionState) { + return `route-template (${name})`; + } + + getDebugCustomRenderTree( + { name, templateName }: RouteTemplateDefinitionState, + state: RouteTemplateInstanceState, + args: CapturedArguments + ): CustomRenderNode[] { + return [ + { + bucket: state, + type: 'route-template', + name, + args, + instance: state.controller, + template: templateName, + }, + ]; + } + + getCapabilities(): InternalComponentCapabilities { + return CAPABILITIES; + } + + didRenderLayout() {} + didUpdateLayout() {} + + didCreate() {} + didUpdate() {} + + getDestroyable(): Nullable { + return null; + } +} + +const ROUTE_TEMPLATE_MANAGER = new RouteTemplateManager(); + +/** + * This "upgrades" a route template into a invocable component. Conceptually + * it can be 1:1 for each unique `Template`, but it's also cheap to construct, + * so unless the stability is desirable for other reasons, it's probably not + * worth caching this. + */ +export class RouteTemplate + implements + ComponentDefinition< + RouteTemplateDefinitionState, + RouteTemplateInstanceState, + RouteTemplateManager + > +{ + // handle is not used by this custom definition + public handle = -1; + public resolvedName: string; + public state: RouteTemplateDefinitionState; + public manager = ROUTE_TEMPLATE_MANAGER; + public capabilities = CAPABILITIES_MASK; + public compilable: CompilableProgram; + + constructor(name: string, template: Template) { + let unwrapped = unwrapTemplate(template); + // TODO This actually seems inaccurate – it ultimately came from the + // outlet's name. Also, setting this overrides `getDebugName()` in that + // message. Is that desirable? + this.resolvedName = name; + this.state = { name, templateName: unwrapped.moduleName }; + this.compilable = unwrapped.asLayout(); + } +} + +// TODO a lot these fields are copied from the adjacent existing components +// implementation, haven't looked into who cares about `ComponentDefinition` +// and if it is appropriate here. It seems like this version is intended to +// be used with `curry` which probably isn't necessary here. It could be the +// case that we just want to do something more similar to `InternalComponent` +// (the one we used to implement `Input` and `LinkTo`). For now it follows +// the same pattern to get things going. +export function makeRouteTemplate( + owner: InternalOwner, + name: string, + template: Template +): CurriedValue { + let routeTemplate = new RouteTemplate(name, template); + return curry(CurriedType.Component, routeTemplate, owner, null, true); +} diff --git a/packages/@ember/-internals/glimmer/lib/renderer.ts b/packages/@ember/-internals/glimmer/lib/renderer.ts index 1c9c0e44469..2e64e171499 100644 --- a/packages/@ember/-internals/glimmer/lib/renderer.ts +++ b/packages/@ember/-internals/glimmer/lib/renderer.ts @@ -31,14 +31,16 @@ import { createConstRef, UNDEFINED_REFERENCE, valueForRef } from '@glimmer/refer import type { CurriedValue } from '@glimmer/runtime'; import { clientBuilder, + createCapturedArgs, curry, DOMChanges, DOMTreeConstruction, + EMPTY_POSITIONAL, inTransaction, renderMain, runtimeContext, } from '@glimmer/runtime'; -import { unwrapTemplate } from '@glimmer/util'; +import { dict, unwrapTemplate } from '@glimmer/util'; import { CURRENT_TAG, validateTag, valueForTag } from '@glimmer/validator'; import type { SimpleDocument, SimpleElement, SimpleNode } from '@simple-dom/interface'; import RSVP from 'rsvp'; @@ -51,6 +53,7 @@ import { EmberEnvironmentDelegate } from './environment'; import ResolverImpl from './resolver'; import type { OutletState } from './utils/outlet'; import OutletView from './views/outlet'; +import { makeRouteTemplate } from './component-managers/route-template'; export type IBuilder = (env: Environment, cursor: Cursor) => ElementBuilder; @@ -370,10 +373,37 @@ export class Renderer { // renderer HOOKS appendOutletView(view: OutletView, target: SimpleElement): void { - let definition = createRootOutlet(view); + // TODO: This bypasses the {{outlet}} syntax so logically duplicates + // some of the set up code. Since this is all internal (or is it?), + // we can refactor this to do something more direct/less convoluted + // and with less setup, but get it working first + let outlet = createRootOutlet(view); + let { name, /* controller, */ template } = view.state; + + let named = dict(); + + named['Component'] = createConstRef( + makeRouteTemplate(view.owner, name, template as Template), + '@Component' + ); + + // TODO: is this guaranteed to be undefined? It seems to be the + // case in the `OutletView` class. Investigate how much that class + // exists as an internal implementation detail only, or if it was + // used outside of core. As far as I can tell, test-helpers uses + // it but only for `setOutletState`. + // named['controller'] = createConstRef(controller, '@controller'); + // Update: at least according to the debug render tree tests, we + // appear to always expect this to be undefined. Not a definitive + // source by any means, but is useful evidence + named['controller'] = UNDEFINED_REFERENCE; + named['model'] = UNDEFINED_REFERENCE; + + let args = createCapturedArgs(named, EMPTY_POSITIONAL); + this._appendDefinition( view, - curry(CurriedType.Component, definition, view.owner, null, true), + curry(CurriedType.Component, outlet, view.owner, args, true), target ); } diff --git a/packages/@ember/-internals/glimmer/lib/syntax/outlet.ts b/packages/@ember/-internals/glimmer/lib/syntax/outlet.ts index debf7a97eb9..bbaedf026c1 100644 --- a/packages/@ember/-internals/glimmer/lib/syntax/outlet.ts +++ b/packages/@ember/-internals/glimmer/lib/syntax/outlet.ts @@ -1,20 +1,22 @@ import type { InternalOwner } from '@ember/-internals/owner'; import { assert } from '@ember/debug'; import { DEBUG } from '@glimmer/env'; -import type { CapturedArguments, DynamicScope } from '@glimmer/interfaces'; +import type { CapturedArguments, DynamicScope, Template } from '@glimmer/interfaces'; import { CurriedType } from '@glimmer/vm'; import type { Reference } from '@glimmer/reference'; import { childRefFromParts, createComputeRef, + createConstRef, createDebugAliasRef, valueForRef, } from '@glimmer/reference'; import type { CurriedValue } from '@glimmer/runtime'; import { createCapturedArgs, curry, EMPTY_POSITIONAL } from '@glimmer/runtime'; import { dict } from '@glimmer/util'; -import type { OutletDefinitionState } from '../component-managers/outlet'; -import { OutletComponentDefinition } from '../component-managers/outlet'; +import { hasInternalComponentManager } from '@glimmer/manager'; +import { OutletComponent, type OutletDefinitionState } from '../component-managers/outlet'; +import { makeRouteTemplate } from '../component-managers/route-template'; import { internalHelper } from '../helpers/internal-helper'; import type { OutletState } from '../utils/outlet'; @@ -56,18 +58,87 @@ export const outletHelper = internalHelper( }); let lastState: OutletDefinitionState | null = null; - let definition: CurriedValue | null = null; + let outlet: CurriedValue | null = null; return createComputeRef(() => { let outletState = valueForRef(outletRef); let state = stateFor(outletRef, outletState); - if (!validate(state, lastState)) { + // This code is deliberately using the behavior in glimmer-vm where in + // <@Component />, the component is considered stabled via `===`, and + // will continue to re-render in-place as long as the `===` holds, but + // when it changes to a different object, it teardown the old component + // (running destructors, etc), and render the component in its place (or + // nothing if the new value is nullish. Here we are carefully exploiting + // that fact, and returns the same stable object so long as it is the + // same route, but return a different one when the route changes. On the + // other hand, changing the model only intentionally do not teardown the + // component and instead re-render in-place. + if (!isStable(state, lastState)) { lastState = state; if (state !== null) { + // If we are crossing an engine mount point, this is how the owner + // gets switched. + let outletOwner = outletState?.render?.owner ?? owner; + let named = dict(); + // Here we either have a raw template that needs to be normalized, + // or a component that we can render as-is. `RouteTemplate` upgrades + // the template into a component so we can have a unified code path. + // We still store the original `template` value, because we rely on + // its identity for the stability check, and the `RouteTemplate` + // wrapper doesn't dedup for us. + let template = state.template; + let component: object; + + if (hasInternalComponentManager(template)) { + component = template; + } else { + if (DEBUG) { + // We don't appear to have a standard way or a brand to check, but for the + // purpose of avoiding obvious user errors, this probably gets you close + // enough. + let isTemplate = (template: unknown): template is Template => { + if (template === null || typeof template !== 'object') { + return false; + } else { + let t = template as Partial