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

Deprecate Route Render APIs #418

Merged
merged 5 commits into from
Feb 15, 2019
Merged

Deprecate Route Render APIs #418

merged 5 commits into from
Feb 15, 2019

Conversation

chadhietala
Copy link
Contributor

@chadhietala chadhietala commented Dec 19, 2018

Rendered

Closes #304.

@rtablada
Copy link
Contributor

I'm a HUGE 👍 on this (though we do have some legacy features that rely on route rendering).

This really simplifies the route/template and rendering relationship across the broader ecosystem. Especially when advising older projects that rely heavily on Route.prototype.render IMO having a dedicated migration guide and deprecating is better than our current silence (and support burden) in documentation.


From a migration strategy I think some migration guides around common scenarios which from experience route rendering can usually be replaced by one of a few strategies:

  1. Match the template name to the route (for the simple case where these got decoupled for some reason)
  2. Create a component and pass in attrs (in the case where a template may have been shared across multiple routes using the render method to populate
  3. Use a parent route and template (often when all children routes have the same UI populated but failed to take advantage of parent routing and outlets)
  4. Use components in a parent route (usually application) with a service tied to it to facilitate state population, changes, and communication
  5. Use wormholes to push components or UI fragments into what is a common use case for render + named outlets

@rtablada rtablada mentioned this pull request Dec 19, 2018
@Gaurav0
Copy link
Contributor

Gaurav0 commented Dec 19, 2018

@chadhietala Rendered link not working any more.

@Gaurav0
Copy link
Contributor

Gaurav0 commented Dec 19, 2018

We are not deprecating Route#templateName property, correct?

@jgwhite
Copy link
Contributor

jgwhite commented Dec 20, 2018

I quickly grep’d the Heroku Dashboard code base to see how we’d be impacted by this. Turns out we use Route#render quite extensively — 38 calls in total.

The vast majority deal with rendering ancillary UI into top-level outlets (e.g. top-nav, action-bar). Using wormholes seems like an appropriate migration path for these.

The other usages are to switch between different templates for an error route. Presumably the appropriate migration path for this is to turn those templates into components.

@chadhietala
Copy link
Contributor Author

@jgwhite yea this was typically the case.

@Gaurav0 I need to check. If not I need to update this RFC and call out that you can still have different route templates.

@knownasilya
Copy link
Contributor

Here's a scenario that uses render/renderTemplate https://github.com/embermap/ember-cli-tailwind/blob/master/app/instance-initializers/ember-cli-tailwind.js#L8, although this might be the wrong tool for the job, but I'm unsure what the right tool would be.

@benedikt
Copy link

benedikt commented Jan 3, 2019

In one of my projects I use renderTemplate and render to replace a sidebar layout of a parent route for certain sub-routes. How would I achieve a similar setup without those methods?

I tried to visualize the situation in the image below. I hope it makes sense in combination with the code snippets.

// router
this.route('authenticated', { path: '' }, function() {
  this.route('campaigns', { resetNamespace: true }, function() {
    this.route('campaign', { path: ':campaign_id' }, function() {
      this.route('messages', function() {
        this.route('message', { path: ':message_id' });
      });
    });
  });
});
// campaign.messages 
renderTemplate() {
  this.render({ into: 'authenticated' });
}

layout

@lifeart
Copy link

lifeart commented Jan 5, 2019

I use renderTemplate for route templates hot-reloading in ember-ast-hot-loader - https://github.com/lifeart/ember-ast-hot-load/blob/master/addon/services/hot-loader.js#L143
If we drop it, we need some way to trigger route template rerendering.

@rmachielse
Copy link

We make a lot of use of render and renderTemplate as well, to render templates in different places. For example dialogs and fullscreen components that can’t be part of the html tree but have to be below it instead. We have a few named outlets for this in application.hbs. I don’t see how this can be replaced with components, as there isn’t just a single one for each outlet but there is a number of components that has to be rendered there depending on the route. The only way for us to solve this would be to allow these things to be rendered in the tree css-wise I guess. In any case this would leave us with a huge architectural question.

@knownasilya
Copy link
Contributor

@rmachielse You would use something like https://github.com/ef4/ember-elsewhere and create a component that has multiple components in it's template (for the multiple component scenario you mentioned).

@rmachielse
Copy link

@knownasilya yep, something like that and ember-wormholes are rightfully alternatives.
Yet I see renderTemplate as a much cleaner approach considering the amount of occurences we have of this. To me this feels like a major core feature whereas those addons feel like workarounds. Those would bloat our templates while renderTemplate keeps those concerns seperated.

@knownasilya
Copy link
Contributor

knownasilya commented Jan 6, 2019

Yeah maybe that is true about it being a type of work around, although things like ember-elsewhere are using public API (see this merged RFC #287) so they are very lightweight.

I do like the how ember-component-routes addon ("routable components" experiment) handles this, feels very clean: https://wongpeiyi.github.io/ember-component-routes/#/rendering. e.g.

import { ComponentRoute } from 'ember-component-routes';

export default ComponentRoute.extend({
  renderComponents() {
    this.renderComponent('app-menu', { outlet: 'sidebar' });
    this.renderComponent('post-page', { into: 'parent-page' });
  }
});

Where parent-page is a route name and the component is rendered into {{component-outlet}} (custom construct).

So maybe an alternative API should be proposed, to at least allow experimentation like above.

@rmachielse
Copy link

That one looks cleaner indeed. However, as long as ember has controllers and routes, renderTemplate looks like a very valid need and I don't see any big advantages in getting rid of it. I do see the complexity argument and I do recognize that this is probably true for smaller applications. However for larger applications like ours, renderTemplate reduces complexity big time, as most developers can keep on developing templates and controllers as always, while the template may be rendered out of the tree. So yes, maybe this is an advanced feature that shouldn't distract beginning developers too much, but it is one that makes our life much easier.

@LevelbossMike
Copy link

LevelbossMike commented Jan 7, 2019

@rmachielse what's the benefit of using renderTemplate over a wormhole like functionality like ember-elsewhere, ember-wormhole or in-element?

fwiw I'm very much in favor of deprecating this as removing the api encourages to use declarative templates that tell a developer where stuff is rendered by just looking at the template files rather than digging through the routes.

@rmachielse
Copy link

@LevelbossMike the separation of concerns is the most valuable thing to me. I think the decision of where to render a template should not be a responsibility of the template itself. Also in our case it allows us to reuse templates, without having to change anything in the template itself.

@rmachielse
Copy link

If this functionality is considered to be too far away from core principles, could it be extracted to an addon? Not one that extends support temporarily but one that can be maintained for longer time? Or does it depend on internal api's too much?

@knownasilya
Copy link
Contributor

knownasilya commented Jan 7, 2019

Deprecation doesn't mean it will be removed right away, so it should be around until Ember 4.0, which is still probably a year away most likely (imo).

@rmachielse
Copy link

Understood, but I would like a future-proof solution.

@chadhietala
Copy link
Contributor Author

chadhietala commented Jan 7, 2019

I have updated the RFC with clearer examples on how to migrate.

@rmachielse: #287 introduced a public API to do what ember-elsewhere / ember-wormhole do. It was created specifically for the cases where you do need to escape the existing DOM hierarchy and render somewhere else. This is great for things like modals, adding to navigation depending on downstream logic, etc. In the cases where you do need access a controller for a specific template and don't want to refactor to a component you can inject controllers onto other objects. The opinion of the core team is that components are the way templates / functionality should be shared and reused.

@Gaurav0 templateName requires another RFC and likely some investigation of migration strategy.

@benedikt
Copy link

benedikt commented Jan 7, 2019

@chadhietala Thanks for adding more examples!

I can see how {{in-element}} would work to replace the use of named outlets. Would that also work to render content in main outlets higher up in the hierarchy?

In your hierarchy escaping example, would it be possible to reliably render into the section#content element, effectively replacing the main outlet, from deeper down in the hierarchy?

It feels a bit weird to replace the contents of the wrapping element, as it feels like it would remove the outlet.

(Please forgive me my ignorance about the details of how {{in-element}} works.)

@lifeart
Copy link

lifeart commented Jan 7, 2019

@chadhietala any public way to force route template rerender without renderTemplate call?

@chadhietala
Copy link
Contributor Author

@lifeart have you seen emberjs/ember.js#17000 and emberjs/ember.js#17001 ?

@lifeart
Copy link

lifeart commented Jan 7, 2019

@chadhietala of course! I made ember-ast-hot-reload addon. And it allow users to hot-reload components (using PoC from ember tests) and route templates, using renderTemplate method. How I can replace it if it will be deprecated?

Code examples above don't deal with route level templates.

@locks locks added the T-framework RFCs that impact the ember.js library label Jan 25, 2019
@chadhietala
Copy link
Contributor Author

chadhietala commented Jan 31, 2019

@Kerrick and @Kilowhisky please take a look at the follow example. I believe both of the concerns are around escaping the hierarchy and I believe this can be solved with {{in-element}} or an addon like ember-eslewhere.

https://ember-twiddle.com/30c6fabaca1f85099eb3bef1f6794f33?openFiles=templates.application.hbs%2Ctemplates.components.nav-for.hbs

@nullrocket
Copy link

I can't say I'm a fan of removing this, others have already expressed how they use it and we use it in a similar way to @Kilowhisky . It works well for us, it is simple to reason about and every other alternative pattern we have tried is a PITA to reason about and has worse rendering performance. I guess we will have to take a stab at the alternatives again.

@rwjblue
Copy link
Member

rwjblue commented Feb 3, 2019

It works well for us, it is simple to reason about and every other alternative pattern we have tried is a PITA to reason about and has worse rendering performance.

I’d like to understand this better. I find the alternatives here massively easier to reason about, what makes them seem overly complicated in your scenarios? I’m also curious how the proposed alternatives would have any different performance at all.

@nullrocket
Copy link

Maybe I should have tried the in-element alternative before I posted the above. We had not tried that pattern, we have been using ember for so long that we settled into our current patterns a couple years ago. I am still finding renderTemplate easier to keep a mental model of but that could be because I am used to our projects being organized that way.

I built two twiddles of the same dummy app, one using renderTemplate one using the suggested {{in-element}} helper. I notice a couple differences so far, using renderTemplate I have a component at the application level template that handles layout, with named outlets I can render just the sections that change when navigating between routes leaving the unchanged things like a common sidebar untouched. I can also use the file system to keep a mental model of where those changed templates are like /route-one/sidebar/template.hbs With the {{in-element}} helper I have to re-render the same sections on each route change.

Maybe if I understood why the change was necessary it would help me be more excited about it. I'm not seeing many other comments saying they want the change, of course the core team probably has more channels than these comments to base a decision on.

Using renderTemplate
https://ember-twiddle.com/c4119b26904fe3f2af7ac04a7e36a6d4?openFiles=application.template.hbs%2Crender-to-element.template.hbs&route=%2Froute-two

Using {{in-element}}
https://ember-twiddle.com/b257d76afae9ed6f04b663d6360cf04a?openFiles=application.template.hbs%2Crender-to-element.template.hbs

@benedikt
Copy link

benedikt commented Feb 3, 2019

Following @nullrocket's example, I tried to implement my renderTemplate use-case with both renderTemplate and {{in-element}}. I didn't quite get it to work in the same way. (Notice how Route B doesn't replace Route A contents).

What am I missing (or doing wrong)?

@LevelbossMike
Copy link

@benedikt as far as I understand in-element is not yet a "real" thing - before it gets to be a real thing i.e. not only an alias to -in-element the behavior will be changed from appending to replacing - reference: https://github.com/emberjs/rfcs/blob/219860c8bf7bf54b4df518fa607329a4c1f17cf6/text/0000-promote-in-element-to-public-api.md#small-proposed-changes

@benedikt
Copy link

benedikt commented Feb 3, 2019

@LevelbossMike Aah, thanks! That explains my issue. Looks like this will work for my use-case indeed. 👍


# How We Teach This

This has not been a mainline API for quite some time now. The guides do not mention this functionality at all. It is likely that the majority of Ember applications do not use these APIs.
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

This has not been a mainline API for quite some time now.

I think this was supposed to read "This API has not existed for very long"?

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'm referring to the fact that renderTemplate and the associated APIs do not and have not played a predominate role in the programming model for quite some time now.

Copy link
Contributor

Choose a reason for hiding this comment

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

This API existed in Ember 1.0, so it definitely "has existed for very long". :)

@mixonic
Copy link
Sponsor Member

mixonic commented Feb 8, 2019

I'd like to suggest a first draft of the steps we need to take to implement this RFC (mostly cribbed from best practice, I don't think we have any of this written down):

  • The {{in-element}} helper implementation remains incomplete. It should be completed.
    • The small changes section of the {{in-element}} RFC needs to be implemented. Specifically the helper should "replace all the content of the destination".
    • The {{in-element}} helper should be documented in the API docs.
    • Adding {{in-element}} usage to the guides can be considered.
  • The Transition Path section of this RFC should be considered a first draft for the deprecation guide.
  • Content for the deprecation message itself needs to be drafted in a PR to Ember.

@chadhietala
Copy link
Contributor Author

@mixonic Can you review the latest changes and confirm that they address your process concerns?


# How We Teach This

These APIs not been a mainline API for quite some time now. The guides briefly mention this functionality. In those cases we should mirgate the guides should link to the `{{in-element}}` documentation and the component documentation. The above "Transition Path" will serve as the deprecation guide.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
These APIs not been a mainline API for quite some time now. The guides briefly mention this functionality. In those cases we should mirgate the guides should link to the `{{in-element}}` documentation and the component documentation. The above "Transition Path" will serve as the deprecation guide.
These APIs not been a mainline API for quite some time now. The guides briefly mention this functionality. In those cases we should migrate the guides should link to the `{{in-element}}` documentation and the component documentation. The above "Transition Path" will serve as the deprecation guide.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

IMO it would be helpful for this sentence to state its subject. I think @chadhietala's wording in his response to be above was just better:

renderTemplate and its associated APIs do not and have not played a predominate role in Ember's common programming model for quite some time. The guides only briefly mention this functionality. In those guides cases that are documented (maybe should link to them here) the guides should link to the {{in-element}} documentation and component documentation as is appropriate.

The "Transition Path" section above will serve as a first draft of the deprecation guide.


# Role Out Plan

Prior to adding the deprecation we must first do the following items
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Prior to adding the deprecation we must first do the following items
We must first do the following items prior to adding the deprecation:


## Unresolved questions

Optional, but suggested for first drafts. What parts of the design are still
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Optional, but suggested for first drafts. What parts of the design are still

## Unresolved questions

Optional, but suggested for first drafts. What parts of the design are still
TBD?
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
TBD?

@mixonic
Copy link
Sponsor Member

mixonic commented Feb 15, 2019

@chadhietala Thank you for the continued cleanup on this after it had theoretically already been approved for merge :-/ Please consider my previous comment about next steps addressed by your changes.

I think you already plan to raise the confusing state of the {{in-element}} public API (nextSibling vs insertBefore) at today's framework core meeting, and IMO resolving that could be an additional item in the TODO list you included in this RFC.

Thanks Chad.

@rwjblue
Copy link
Member

rwjblue commented Feb 15, 2019

I think you already plan to raise the confusing state of the {{in-element}} public API (nextSibling vs insertBefore) at today's framework core meeting

Yep, the confusion is cleared up on that front.

@rwjblue
Copy link
Member

rwjblue commented Feb 15, 2019

Thanks for iterating on this @chadhietala / @mixonic!

I think this is in a good place to land (only a few weeks after we meant to 😸)...

@rwjblue rwjblue merged commit 0a4f39a into master Feb 15, 2019
rwjblue added a commit that referenced this pull request Feb 15, 2019
@mixonic
Copy link
Sponsor Member

mixonic commented Feb 15, 2019

Yep, the confusion is cleared up on that front.

Haha @rwjblue can you drop a link or summarize to break the suspense?

@mixonic mixonic deleted the deprecate-route-render-apis branch February 15, 2019 22:42
@rwjblue
Copy link
Member

rwjblue commented Feb 15, 2019

@mixonic hmmm, I don't follow, you laid out the specifics in #418 (comment):

  • The {{in-element}} helper implementation remains incomplete. It should be completed.

    • The small changes section of the {{in-element}} RFC needs to be implemented. Specifically the helper should "replace all the content of the destination".
    • The {{in-element}} helper should be documented in the API docs.
    • Adding {{in-element}} usage to the guides can be considered.

You were totally right, {{in-element will have replace by default semantics, and to get "append" (currently what {{-in-element does) you will need to specify insertBefore=null. At todays core team meeting we just confirmed that was what we intended.

@rwjblue
Copy link
Member

rwjblue commented Feb 15, 2019

FWIW, @chadhietala is working on those changes over in glimmerjs/glimmer-vm#918.

@mixonic
Copy link
Sponsor Member

mixonic commented Feb 15, 2019

@rwjblue right, when I say:

the confusing state of the {{in-element}} public API (nextSibling vs insertBefore)

I'm definitely talking about that nextSibling is/was the actual implemented API in Glimmer and Ember, but the RFC to promote {{in-element}} only mentioned insertBefore. I see in today's core team notes:

RJ: Concretely, forget about next-sibling, only support insert-before. It’s a better semantic naming.

So the item I was suggesting could be added or at the least should be tracked is that glimmerjs/glimmer-vm#918 and the sibling PR to Ember should be migrating to insertBefore and dropping nextSibling (I'm not sure if it needs intimate API deprecation or not).

igorT pushed a commit to igorT/rfcs that referenced this pull request Jun 29, 2019
@IBue
Copy link

IBue commented Oct 22, 2019

To me, the renderTemplate/render combo inside a Route actually has a very declarative character. It enables VERY easily mounting a nested details-route's template-subtree somewhere up in the template hierarchy (as in @benedikt 's case) or even into the application template via main {{outlet}}s for some sort of intermediate 'fullscreen' view of e.g. editing forms without any specific CSS intervention or concern about data context.
Hopefully, the lately specified replace-innerHtml behaviour of {{in-element}} will provide an almost 1:1 transition path by the following pattern:

target route template: <div id="targetSlotX">{{outlet}}</div>
source route template: {{#in-element targetSlotX}}…{{outlet}}…{{/in-element}}

@NullVoxPopuli
Copy link
Sponsor Contributor

@IBue, you may be interested in: #499

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Final Comment Period T-framework RFCs that impact the ember.js library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate Route#render/Route#renderTemplate