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

Support Populating head tags in Routes #506

Closed
wants to merge 3 commits into from

Conversation

RobbieTheWagner
Copy link
Member

@RobbieTheWagner RobbieTheWagner commented Jun 21, 2019

@makepanic
Copy link
Contributor

makepanic commented Jun 24, 2019

Should it be possible to invoke the head method again to update head parameters?
If so, how would a developer do that?

Maybe we should define head to be a computed property to allow invalidation.

Another thing one could discuss is how nested routes should behave.
Does the head change if it's defined in a child route? Will it somehow merge up the route tree?

Given routes root > list > detail how would head behave if head is defined in root and detail?

e.g. https://www.npmjs.com/package/ember-cli-document-title-northm will merge up to allow easy title combinations.

@RobbieTheWagner
Copy link
Member Author

Should it be possible to invoke the head method again to update head parameters?
If so, how would a developer do that?

I would be inclined to say no. Is there a use case for updating meta later after rendering the page?

Another thing one could discuss is how nested routes should behave.
Does the head change if it's defined in a child route? Will it somehow merge up the route tree?

I think for sure you should be able to define a title template, but the other meta I could go either way on.

@makepanic
Copy link
Contributor

makepanic commented Jun 24, 2019

Is there a use case for updating meta later after rendering the page?

I was mostly thinking about it, because the meta method returns title which will probably update multiple times during the apps lifetime.

@RobbieTheWagner
Copy link
Member Author

@makepanic

I was mostly thinking about it, because the meta method returns title which will probably update multiple times during the apps lifetime.

This hook will be in each route, so there is no need to make it computed or call it again, it will be set per route already.

@jherdman
Copy link

I've used a few of the community addons for setting and updating the <title tag, and I think it's long overdue that we have something like this baked into core. Something that doesn't seem addressed by your RFC is some of the specific details of how this works. Some questions:

  • Most community addons have the idea of sub-routes, or sub-templates, appending or prepending to the <title. Is that the case here? If so, how would this work?
  • <title seems like a very special case for a template concern (i.e. it's kinda sorta global). Does it really make sense to put this concern on a route?
  • It's not clear to me if meta is a special key that's returned, or if it's one of ANY tags I could specify. E.g. could I specify link, or how about script?
  • Will hid duplicates cause errors?
  • I'm not saying this is necessarily a great idea, but dynamically updating <title is a thing: https://output.jsbin.com/togitukopa/1. Maybe it's an edge case we don't want to support now, but it may be worth considering.

@RobbieTheWagner
Copy link
Member Author

Most community addons have the idea of sub-routes, or sub-templates, appending or prepending to the <title. Is that the case here? If so, how would this work?

That was mentioned above. It'll work just like you expect it to, allowing for templates of some sort, like the addons do.

<title seems like a very special case for a template concern (i.e. it's kinda sorta global). Does it really make sense to put this concern on a route?

I think it does. title is set at the route level with the existing addons, and in other frameworks. I think the use cases for changing any head data, after the page is fully rendered, are pretty minimal. Is there a use case you have in mind for wanting to change it multiple times for a single page?

It's not clear to me if meta is a special key that's returned, or if it's one of ANY tags I could specify. E.g. could I specify link, or how about script?

I think, in theory, we should support all relevant tags, but starting with meta would be a good first step.

@makepanic
Copy link
Contributor

Is there a use case you have in mind for wanting to change it multiple times for a single page

Usually any messenger app will update its page title for new activity.

@RobbieTheWagner
Copy link
Member Author

Usually any messenger app will update its page title for new activity.

Ah gotcha, good point. We probably should provide a way to dynamically update it then. Thanks for giving that example!

@lupestro
Copy link
Contributor

For some uses of head tag substitutions, especially for things like SEO or accessibility, they don't do you a lot of good unless they are in the page as it first hits the browser because the scanners only execute at the usual page load complete events. That usually means the substitutions with this restriction have no effect unless they hit in prember or fastboot. (Of course using routes with a11y is its own story...) You might want a paragraph about where this won't help.

@rtablada
Copy link
Contributor

Pinning this down to something in routes specifically feels like a side step move.

Instead of making this a route API what if we made head tag manipulation an official and injectable service.

This allows a lot more flexibility meaning users don't have to learn a new API or hook in the route. Instead it becomes a standard service users can interact with via Routes, Components, and Modifiers.

I'm inspired a lot by Next.js's use of Head as a context provider (similar to services) which makes it really easy to work with as a standard part of the app (and also really easy to not have be part of your app if you don't want to handle head manipulation).

@NullVoxPopuli
Copy link
Sponsor Contributor

NullVoxPopuli commented Jun 26, 2019

Instead of making this a route API what if we made head tag manipulation an official and injectable service.

I'm a big fan of services.
I think a head service may align more with the idea that ember is first and foremost a component<->service framework

@rtablada
Copy link
Contributor

For some examples and reasoning, making this a service unlocks:

  • Use in components (possibly even an official <Head></Head> component)
  • Use in routes
  • Use in services
  • Easy test mocking with no extra tooling
  • Modifier use

For example if I have a <BreadCrumb /> component the template for BreadCrumb could have something in the template like this:

<li {{did-insert this.addBreadCrumbToHead}} {{will-destroy this.removeBreadCrumbFromHead}}>

@RobbieTheWagner
Copy link
Member Author

@rtablada @NullVoxPopuli I'm in favor of a service, but I would want to have it injected in routes by default, I think. We could probably work out an API that would work the same regardless of where the service was injected, but the idea would be to set things primarily for pages/routes.

I believe Nuxt.js, which may or may not be similar to Next.js, allows setting head data wherever you want, but the API is the same for global, pages, components, etc just a head() hook in each thing. We could provide a service that sets up that API for each thing. i.e. run on didInsertElement for components or activate for routes, all dependent on what is consuming the service.

@tleperou
Copy link

I'm in favor of a service, but I would want to have it injected in routes by default

The recent initiatives go to an explicit injection. The initializers remain to inject into routes the dependencies of your choice.

As you, I am a huge fan of services. It would meet your expectations @rwwagner90, providing clarity and flexibility.

@RobbieTheWagner
Copy link
Member Author

@tleperou it's just one extra step that people have to do, when I would prefer it to feel built in. Maybe there is a way we can inject on the fly versus always injecting in routes. i.e. only inject if head() is used.

@NullVoxPopuli
Copy link
Sponsor Contributor

What if the default injections are added to the route blueprint?

@RobbieTheWagner
Copy link
Member Author

What if the default injections are added to the route blueprint?

I tend to prefer as much magic as possible, but others may prefer more explicit injections.

@tleperou
Copy link

Even if in the case of the title, for me, it makes sense to get within the route, I would choose the explicitness over the implicitness. Each one his own, though.

It's not clear to me if meta is a special key that's returned, or if it's one of ANY tags I could specify. E.g. could I specify link, or how about script?

However, inject script/link from there seems inappropriate. Meta tags do not alter the execution of the application while scripts and CSS may do it. The risk I see is an unpredictable behavior.

@rtablada
Copy link
Contributor

While I do get the ease of use argument the tools are there to choose your own ease of use. I think it'd be much easier to come to a consensus on an injectable service rather than adding to the already large API surface of routes.

For those that want to add their own special head method or something else, creating a decorator or base route with afterModel or something similar would be fairly easy to make that decision and codify for your team.

I also think that a service would be a first step before recommending adding new hooks and magic to routes.

@RobbieTheWagner
Copy link
Member Author

I also think that a service would be a first step before recommending adding new hooks and magic to routes.

There is no disagreement here. Service based is what everyone has asked for here, any and all magic would be in the service itself. Just trying to find ways to make everyone happy here 😃

@snewcomer
Copy link
Contributor

I'm very much in favor of this. Would also add that it has to be flexible enough for a variety of types of apps. Could even be a top level component. A few non traditional examples would include:

  1. optimistic routes where data might not be immediately available or resolved at the route level
  2. A component/presenter that has to normalize data post route activation.

If this never makes it out of RFC, I would be in favor of helping out including how one could do this in the docs as well.

@RobbieTheWagner
Copy link
Member Author

@snewcomer I need to update this RFC based on the comments so far. It has stalled out for a bit.

@wagenet
Copy link
Member

wagenet commented Jul 23, 2022

@rwwagner90 do you think you're likely to update this RFC? Or would anyone else like to take it over?

@RobbieTheWagner
Copy link
Member Author

@wagenet is there interest in still moving this forward? I could update it if there is.

@chriskrycho
Copy link
Contributor

@rwwagner90 we discussed this at Framework Core today and agreed that while we think this is definitely a problem worth solving, we don’t want to solve it by adding more behavior to the existing routing stack. We are, as part of Polaris, looking at how to re-work routing in general, so adding more things that has to solve seems like a bad move.

Accordingly, we're putting this in FCP to close today.

If you’re still interested in driving forward the solutions here, there are a couple options we’re thinking about in this space:

  • handling it via a Resource which does something kind of like how the {{page-title}} helper works
  • building something which can actually just fully manage (some subset of?) the <head> section (it's pretty complicated to get that right, given that that section needs to be able to do things like… load Ember)

There might be others!

@chriskrycho chriskrycho added the FCP to close The core-team that owns this RFC has moved that this RFC be closed. label Sep 16, 2022
@RobbieTheWagner RobbieTheWagner deleted the populate-head-tags branch October 2, 2022 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FCP to close The core-team that owns this RFC has moved that this RFC be closed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants