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

Template Tag In Routes #1046

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Template Tag In Routes #1046

wants to merge 5 commits into from

Conversation

ef4
Copy link
Contributor

@ef4 ef4 commented Oct 4, 2024

Propose Use Template Tag in Routes

Rendered

Summary

This pull request is proposing a new RFC.

To succeed, it will need to pass into the Exploring Stage), followed by the Accepted Stage.

A Proposed or Exploring RFC may also move to the Closed Stage if it is withdrawn by the author or if it is rejected by the Ember team. This requires an "FCP to Close" period.

An FCP is required before merging this PR to advance to Accepted.

Upon merging this PR, automation will open a draft PR for this RFC to move to the Ready for Released Stage.

Exploring Stage Description

This stage is entered when the Ember team believes the concept described in the RFC should be pursued, but the RFC may still need some more work, discussion, answers to open questions, and/or a champion before it can move to the next stage.

An RFC is moved into Exploring with consensus of the relevant teams. The relevant team expects to spend time helping to refine the proposal. The RFC remains a PR and will have an Exploring label applied.

An Exploring RFC that is successfully completed can move to Accepted with an FCP is required as in the existing process. It may also be moved to Closed with an FCP.

Accepted Stage Description

To move into the "accepted stage" the RFC must have complete prose and have successfully passed through an "FCP to Accept" period in which the community has weighed in and consensus has been achieved on the direction. The relevant teams believe that the proposal is well-specified and ready for implementation. The RFC has a champion within one of the relevant teams.

If there are unanswered questions, we have outlined them and expect that they will be answered before Ready for Release.

When the RFC is accepted, the PR will be merged, and automation will open a new PR to move the RFC to the Ready for Release stage. That PR should be used to track implementation progress and gain consensus to move to the next stage.

Checklist to move to Exploring

  • The team believes the concepts described in the RFC should be pursued.
  • The label S-Proposed is removed from the PR and the label S-Exploring is added.
  • The Ember team is willing to work on the proposal to get it to Accepted

Checklist to move to Accepted

  • This PR has had the Final Comment Period label has been added to start the FCP
  • The RFC is announced in #news-and-announcements in the Ember Discord.
  • The RFC has complete prose, is well-specified and ready for implementation.
    • All sections of the RFC are filled out.
    • Any unanswered questions are outlined and expected to be answered before Ready for Release.
    • "How we teach this?" is sufficiently filled out.
  • The RFC has a champion within one of the relevant teams.
  • The RFC has consensus after the FCP period.

@github-actions github-actions bot added the S-Proposed In the Proposed Stage label Oct 4, 2024
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

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

:shipit:


### ember-route-template addon

This RFC replaces the [ember-route-template](https://github.com/discourse/ember-route-template) addon. If you're already using it, it would continue to work without breaking, but you can simply delete all the calls to its `RouteTemplate` function and remove it. The popularity of that addon among teams who are already adopting Template Tag is an argument in favor of this RFC.
Copy link
Contributor

Choose a reason for hiding this comment

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

+100 and thanks for bringing this into Ember directly!

@chancancode
Copy link
Member

Broadly 👍🏼

I think we should also:

  1. add an optional feature that changes the meaning of bare .hbs files in the route templates folder to mean a template only component (as opposed to a component-ish but with controller as this – you can still access it easy enough with @controller)
  2. default to disabled with a deprecation to set the flag one way or the other
  3. eventually deprecate disabling the feature

This is similar to the application template wrapper optional feature/transition. For most apps, the recommendation shouldn't be just setting the feature to disabled, but to actually convert the templates to components – just change this to @controller and keep the .hbs extension (going to .gjs would require analyzing imports etc and is better left to a dedicated codemod). @ember/optional-feature has some rudimentary codemod capabilities that is probably just good enough to take care of this (assuming no implicit this, etc).

The motivation behind this is to more strongly align with the component model everywhere else. In the component folder, if you have a bare .hbs file these days, it just means a template-only component. It would be quite confusing for it mean a wildly different thing in the route template folder. Plus, as it is implemented right now – if you add a .js file next to the .hbs file, because of build-time co-location, it will also turn it into a component.

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Oct 4, 2024

I don't think we should touch the existing hbs behavior, even with a flag -- benefit too little

@ef4 ef4 added the S-Exploring In the Exploring RFC Stage label Oct 4, 2024
@ef4
Copy link
Contributor Author

ef4 commented Oct 4, 2024

add an optional feature that changes the meaning of bare .hbs files in the route templates folder

I think the hard part about this is that there are quite a lot of places that have to make assumptions about which folders are allowed to do template colocation. We're likely to be chasing down bugs for a while. A partial list:

  • embroider
  • ember-auto-import
  • ember-cli-htmlbars
  • glint
  • ember-template-lint

@chancancode
Copy link
Member

I think the hard part about this is that there are quite a lot of places that have to make assumptions about which folders are allowed to do template colocation. We're likely to be chasing down bugs for a while. A partial list:

I guess I was assuming that some/all of these tools don't care about folders (some may not even have the concept about folders), and blindly assumes adjacent .js/.hbs = component. In my mind that would be the simpler thing to do (considering pods layout also), and arguably the intention of the co-location RFC, but empirically you may be right that they do care somewhat.

@chancancode
Copy link
Member

Also, I was mostly coming from thinking about the implementation in ember-source, we have access to the flag at runtime and can implement the proposed behavior without build tool support. But I do think you are correct that we need to consider whether it will work out in language servers, etc.

@ef4
Copy link
Contributor Author

ef4 commented Oct 4, 2024

Yeah lots of stuff actually blows up if you blindly assume colocation. The most problematic case is not when both js and hbs are present, but when only hbs is.

Also, I was mostly coming from thinking about the implementation in ember-source, we have access to the flag at runtime and can implement the proposed behavior without build tool support.

This would be a radical departure from how it works today and I would not be in favor. Template colocation is an entirely build-time phenomenon today. ember-source knows nothing about it.

@chancancode
Copy link
Member

Also, I was mostly coming from thinking about the implementation in ember-source, we have access to the flag at runtime and can implement the proposed behavior without build tool support.

This would be a radical departure from how it works today and I would not be in favor. Template colocation is an entirely build-time phenomenon today. ember-source knows nothing about it.

I will be more precise about what I meant:

My assumption (apparently not entirely accurate) was that the current build tools more-or-less blindly see co-located pairs and smoosh them down to a single js module, but otherwise leave single hbs files alone. Then, at runtime, if ember-source encounters a template module it'll, at runtime, convert it to a template-only component.

I am pretty sure this was at one point how it had to work, since that behavior was an optional feature for transitional purposes (similar to here), but perhaps that has changed since the flag has been removed. We still seem to have kept the code around in ember-source though. Not sure how that is managed in the build tool side, maybe they also stopped supporting older versions with the flag?

Anyway I didn't mean to say that we shouldn't do this in build tools. In fact I was assuming that's what we already do (when there is a pair). What I mean is that if the build tool didn't take care of the lone hbs files (and my assumption was that they don't today, thus requiring an upgrade), we can still make it work at runtime without hard-requiring build-tool upgrades which we typically don't do.


In any case, I can't really say that much about how hard/easy it is to implement across the ecosystem, but I do think it is an attractive proposition that should be seriously considered, and if rejected, the drawbacks noted in the RFC.

If we do this, then we have a consistent behavior across the board – .hbs means template-only components everywhere, .js/.ts+.hbs works everywhere, and .gjs/.gts works everywhere as well. We also entirely eliminate the concept of standalone templates in the happy path from a teaching perspective. Ultimately, this should also make tooling easier to implement, but I agree there may be some complexity in the meantime if they need to support both.

The drawbacks of not doing this is a bunch of odd inconsistencies – as mentioned the divergent behavior between what lone .hbs file means is one thing, but also, if the build tools doesn't do what I assumed, then we also have to define what it means when you have a .js+.hbs pair in this context, at least making it an error.

Ultimately, I think this fits so well into the new programming model that it's only natural for someone to try the usual patterns in the templates folder, and the failure mode is quite surprising (and subtle).

Keeping these divergent behavior also complicate things for tools in the long run, so it's ultimately a balancing act. I think either way we are going to be chasing down bugs for a while – because of the odd behavior of route templates the "natural" handling wouldn't work for those (e.g. if you are writing a new codemod, if you just write one set of behavior it likely won't work for route templates). This basically gives us a path to get off that weird behavior by running a one time codemod and be done with it, and new tools can require to be already done and not have to worry about the divergent behavior. But I can be convinced that we think we already hunted down most bugs in that direction and would rather leave it alone.


Because the actual difference we are talking about is very small, another alternative to explore would be to make it work both ways with deprecation:

  1. {{@controller}} always work
  2. this refers to the instance if it's a class-based component
  3. Otherwise, this refers to the controller but with a deprecation

This eliminates the need for a flag, and you can always treat route templates as components. It's just that if you happen to have legacy route templates that refers to this we won't break it yet.

I haven't thought that much about how it would be implemented on the ember-source side, but I think if we are willing to put up with some dirty dark magic for a little bit (hidden inside ember-source/glimmer-vm), it can be done regardless of what the build tool does.

@chancancode
Copy link
Member

Put it a different way: the proposal is to stop treating the route templates folder as special in any way.

In the abstract, that should be a attractive proposition, both for learning and for implementation reasons, it also eliminates another place where file placement is significant (and so you/tools have to care about it)

The obstacles seem to be that currently tools do have to sometimes special case it, and is already implanted to behave that way.

If we can, I think it’s worth exploring in the direction of what can we do here to make it possible to opt out of those special treatment. It does take some work to update the existing implementation, but it would be in the direction of where we want to end up anyway.

@kategengler
Copy link
Member

I think making @controller work in route templates and potentially deprecating the use of this in .hbs route templates is a potentially helpful step for migration.

Some things to remember come implementation time:

  • The text on the component needs updating
  • ember-template-imports is not yet in the blueprint

@bwbuchanan
Copy link

I'm probably missing a bunch of context here, but why couldn't a Route specify the template instead of putting it in a separate .hbs or .gts file?

e.g.

import Route from 'component-route';

interface HelloModel {
  world: string;
}

export default class Hello extends Route<HelloModel> {
  <template>Hello {{@model.world}}!</template>

  override model(): HelloModel {
    return { world: 'Earth' };
  }
}

I have this almost working using something similar to the code below.

// component-route/index.ts

import { getComponentTemplate } from '@ember/component';
import { getOwner } from '@ember/owner';
import Route from '@ember/routing/route';

export default class ComponentRoute<Model = unknown> extends Route<Model> {
  override _setRouteName(name: string): void {
    super._setRouteName(name);
    const template = getComponentTemplate(this.constructor);
    if (template != undefined) {
      getOwner(this)?.register(`template:${name}`, template);
    }
  }
}

The only problem is that this in the route template references an empty controller instead of the route instance, so you can't do the below without an intermediate component:

...

goHome() {
  this.router.transitionTo('index');
}

<template>
    <button {{on 'click' this.goHome}}>Go Home</button>
</template>

@ef4
Copy link
Contributor Author

ef4 commented Oct 10, 2024

why couldn't a Route specify the template instead of putting it in a separate .hbs or .gts file?

That would be an example of what I meant by:

A "Route Manager" RFC would allow the creation of new Route implementations that could have their own opinions about how to route to GJS files. This RFC does not preclude that other work from also happening.

I definitely want us to do that too. It's just demonstrably a lot more complex, which is why it isn't done yet. My intent with this RFC is to do a thing that we can ship immediately by targeting one very specific spot in the implementation.

An example like the one you showed would require a new Route base class. But if we commit to a new Route base class, we're really going to want it to solve not just the problem of how to route to GTS, but also the problems of having the correct lifetime, fully eliminating controllers (which means designing better query params), allowing data-loading hooks to run in parallel, eliminating confusing old behaviors around parameter serialization, and ideally also providing strong typescript types for routes. All while correctly inter-operating with today's Route and its quirky timing and transition behaviors, since people can't be expected to convert all their routes simultaneously.

Also, the designs people tend to intuitively reach for here all make the problem of route-based code-splitting worse by putting the data-fetching code and the component hierarchy in the same module, where they can't be split apart without JS-spec-violating behaviors. I am still hoping our design will allow the data for a route and the component hierarchy for a route to load in parallel, but that is a point where reasonable people can disagree (and hence why "Route Manager" would be the first stop, since like Component Manager it would let people do their own implementations on stable public API to demonstrate how they think routing should work).

@ef4
Copy link
Contributor Author

ef4 commented Oct 10, 2024

It does take some work to update the existing implementation, but it would be in the direction of where we want to end up anyway.

But it's not where we want to end up. We want to end up with no more .hbs files!

I really, really do not want to spend time going around the ecosystem conditionally changing how .hbs files are interpreted. They are a legacy codepath. They should be kept as stable as possible.

We still seem to have kept the code around in ember-source though

That code doesn't do component template colocation. It can't. It only covers the case where app/templates/components/thing.hbs exists and has no corresponding app/components/thing.js.

The ember-resolver stores modules without file extensions. It cannot distinguish app/components/thing.js from app/components/thing.hbs. That is why colocation has always been a build-time feature. The resolver only ever sees the JS modules, the build-tooling has taken care of synthesizing it out of one or both of the authored files.

@Panman82
Copy link

While I think this is a good step, I also think this is getting mixed-in with interest for other router improvements. Last I heard there was work taking place for Polaris Edition, but perhaps the router stuff stalled.?. If there was a place to see what was planned for that, it would help to see how this RFC will lead into other router improvements.

@ef4 ef4 removed the S-Proposed In the Proposed Stage label Oct 18, 2024
@kategengler
Copy link
Member

While I think this is a good step, I also think this is getting mixed-in with interest for other router improvements. Last I heard there was work taking place for Polaris Edition, but perhaps the router stuff stalled.?. If there was a place to see what was planned for that, it would help to see how this RFC will lead into other router improvements.

While we'd ideally have at least a Route Manager RFC for Polaris, for now, this RFC is the "router stuff" for Polaris. There is a lot of things we could do but for the most part our efforts are elsewhere and this is a small change that will enable a more coherent story.

This came about while I was pairing with @ef4 on how to update the guides for Template tag. It is incredibly weird to explain .hbs for some things and .g(j|t)s for something else. Ed mentioned that he uses and recommends discourse/ember-route-template. When core team members are building apps and recommending different paths than our guides follow, it is a giant flashing sign that we need to do something to bring those changes to users following the documentation.

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Oct 18, 2024

With this change,

  • no hbs is needed ever
  • we can use @glint/*@unstable (which currently doesn't support split-file components, because it's hard) (this version of Glint also solves a lot of folks (valid) complaints about Glint ergo)

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-Exploring In the Exploring RFC Stage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants