Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow GJS/GTS route templates #20768

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Allow GJS/GTS route templates #20768

wants to merge 4 commits into from

Conversation

ef4
Copy link
Contributor

@ef4 ef4 commented Oct 2, 2024

In the process of starting to write guides content for template tag, @kategengler and I decided it's too clunky to force new learners to mix HBS and GJS.

In practice, experienced ember devs who are adopting template tag are often using ember-route-template so they can author routes in template tag. We considered just proposing ember-route-template for the default blueprint, but then realized that in that case it's actually just much better to build the same feature into ember.

AFAIK, this PR is the full implementation and it's ready to go, pending an RFC.

This does not preclude also doing a more comprehensive Route Manager API that would cleanup more about routes. But this seems like an important smallest step we can make today to improve the experience.

In the process of starting to write guides content for template tag, @kategengler and I decided it's too clunky to force new learners to mix HBS and GJS.

In practice, experienced ember devs who are adopting template tag are often using [ember-route-template](https://github.com/discourse/ember-route-template) so they can author routes in template tag. We considered just proposing ember-route-template for the default blueprint, but then realized that in that case it's actually just much better to build the same feature into ember.

AFAIK, this PR is the full implementation and it's ready to go, pending an RFC.

This does not preclude also doing a more comprehensive Route Manager API that would cleanup more about routes. But this seems like an important smallest step we can make today to improve the experience.
@NullVoxPopuli
Copy link
Contributor

I am a fan, and this reduces the divergences in the TS support PR for the v2 app blueprint

@ef4
Copy link
Contributor Author

ef4 commented Oct 2, 2024

cc @chancancode: this would essentially copy your ember-route-template implementation into ember.

@chancancode
Copy link
Member

Yea, I think it definitely should be built-in and the wrapper shouldn't be necessary. Mostly just have to decide whether we are happy enough with @model and @controller for now, and if those somehow locks us into an undesirable path. Couldn't think of any obvious problems off the top of my head, generally +1.

'import/no-unresolved': [
'error',
{
ignore: ['@ember/template-compiler'],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a build-time feature that we added in RFC 931 but haven't used internally in ember-source yet. The whole implementation of it lives in babel-plugin-ember-template-compilation.

@@ -46,6 +46,7 @@ export interface OutletDefinitionState {
template: Template;
controller: unknown;
model: unknown;
component: object | undefined;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The choice of type here reflects that a component is anything with a component manager, and having a component manager requires being a weakmap key.

@@ -89,6 +89,9 @@ export const outletHelper = internalHelper(
return model;
});

let componentRef = childRefFromParts(outletRef, ['render', 'component']);
named['component'] = componentRef;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike model above, this argument doesn't change for the lifetime of this render state. Its lifetime is the same as outletRef.render.template, rather than outletRef.render.model.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it doesn't change then something like createConstRef(state.component, '@component') should do it. It'll result in less bounds tracking for the DOM etc

this.route('first');
this.route('second');
});
this.add('template:first', template('First'));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This template is the JS version of our <template></template>, so the value here is a component.

@@ -1825,6 +1826,15 @@ export function getRenderState(route: Route): RenderState | undefined {
return route[RENDER_STATE];
}

import { precompileTemplate } from '@ember/template-compilation';

const RoutableComponent = precompileTemplate(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to name it this for the LOLs.

@ef4
Copy link
Contributor Author

ef4 commented Oct 4, 2024

RFC: emberjs/rfcs#1046

@@ -89,6 +89,9 @@ export const outletHelper = internalHelper(
return model;
});

let componentRef = childRefFromParts(outletRef, ['render', 'component']);
named['component'] = componentRef;

if (DEBUG) {
named['model'] = createDebugAliasRef!('@model', named['model']);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also not really sure what's up with this, does this just predate createComputeRef taking an optional label argument? (In any case the positioning of the new code split these two related chunks)

@chancancode
Copy link
Member

It is on my personal TODO list, but beyond what I commented above, I suspect we can simplify this code quite a bit (and clearly mark the legacy code path) by flipping it around – have the happy path always just assume and invoke a component with @controller and @model, and wrap/upgrade any legacy templates as a special component as needed.

chancancode added a commit that referenced this pull request Nov 22, 2024
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 <[email protected]>
chancancode added a commit that referenced this pull request Nov 22, 2024
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants