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

Add `load` hook to Route #510

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@michaelrkn
Copy link

commented Jul 1, 2019

This is almost entirely based off a blog post by @chadhietala. Thanks to @NullVoxPopuli for putting together #499 and reviewing the first draft of this.

Rendered

michaelrkn added some commits Jun 28, 2019

@jessica-jordan jessica-jordan referenced this pull request Jul 2, 2019

Merged

The Ember Times No. 105 - July 5th 2019 #164

8 of 11 tasks complete
Show resolved Hide resolved text/0000-route-data-loading.md Outdated
Show resolved Hide resolved text/0000-route-data-loading.md
@tleperou

This comment has been minimized.

Copy link

commented Jul 3, 2019

The Guides would need to be updated to use load() instead of model(), beforeModel(), and afterModel().

Is there a replacement of beforeModel and afterModel? They are frequently used to grant access (or not) to routes.

@tleperou

This comment has been minimized.

Copy link

commented Jul 3, 2019

Put into the perspective of the of this proposal, controller-migration-path and the explicitness' trend*, may I wonder myself on the impact and the adoption of the load hook?

It would make sense to get in fine under /routes basic components. Hence, render modifiers can be explicitly performed to handle the model, beforeModel, afterModel, activate, deactivate hooks. This would reproduce the same behavior of Controllers and Routes, right?

(*) Add queryParams to the router service, Explicit Service Injection, probably others

@michaelrkn

This comment has been minimized.

Copy link
Author

commented Jul 4, 2019

Is there a replacement of beforeModel and afterModel? They are frequently used to grant access (or not) to routes.

@tleperou The intent is that such behavior could be taken care of in the load hook. I will be more explicit about this.

It would make sense to get in fine under /routes basic components. Hence, render modifiers can be explicitly performed to handle the model, beforeModel, afterModel, activate, deactivate hooks. This would reproduce the same behavior of Controllers and Routes, right?

I'm not totally clear on your question here, sorry!

michaelrkn added some commits Jul 4, 2019

Show resolved Hide resolved text/0000-route-data-loading.md Outdated
Show resolved Hide resolved text/0000-route-data-loading.md Outdated

michaelrkn and others added some commits Jul 5, 2019

Add class name to route code snippet
Co-Authored-By: Eli Flanagan <efx@users.noreply.github.com>
Add class name to route code snippet (pt 2)
Co-Authored-By: Eli Flanagan <efx@users.noreply.github.com>
@tleperou

This comment has been minimized.

Copy link

commented Jul 6, 2019

@michaelrkn, I think that render modifiers can replace the hooks, with those equivalents:

  • activate, beforeModel, model: will-render (illustrated below)
  • deactivate: will-destroy

Then, routes render a component, only (simplifying the APIs).

It may be out of the scope of this RFC, though 😅

{{! templates/profile.hbs }}
<div {{will-render (perform this.load)}}>
  {{! ... }}
</div>
// routes/profile.js
@service store;
@service queryParams;

@task load = function*() {
  const { profile_id } = this.queryParams 
  try {
    this.profile = yield this.store.findRecord('profile', profile_id);
    if(this.profile.admin) {
      this.router.transitionTo('accessDenied');
    }
  } catch(e) {
    // ...
  }
}
@michaelrkn

This comment has been minimized.

Copy link
Author

commented Jul 6, 2019

@tleperou Interesting! I don't think I'm qualified to weigh in on the advantages of loading data from the route versus from the component, but I'd be happy to close this if people more qualified than I am believe that's the best way forward.

@webark

This comment has been minimized.

Copy link

commented Jul 6, 2019

For me, the main advantages of a route, and loading data from the route, is the clear communication it provides. If i’m encounter a bug, or i want to make additions to a feature, even if i have never worked in a project before, i can go to the router, see what route i’m on, then go to the route (and it’s heirchy), and find out everything that’s been loaded up, all the data i have available, and have a pretty good understanding of how the data is handled in the app. (which is a reason why i am a fan of route actions too, but that’s another battle).

Whenever we have moved a chunk of loading to the components (generally due to ember concurrency) it has always been a source of confusion and incoherent patterns. (and often added to these insert elements, inits, will render hooks, observers 🤢)

Even though sometimes simpler and potentially even more elegant solutions can be devised with putting the loading directly on a component, you sacrifice that clear communication.

The thing i enjoy about moving from “model” to “load” is that it is more explicit about what is happening, it’s easier to explain to newcomers, and it avoids the awkward feeling when you load multiple items into one model hook, and then need to set aliases in the controller.

the requiring of an object i don’t love. There are many times where i just need a single item in a route. Something like a default of “data” or an equivalent could be nice. (i could see some awkward instances where someone returns an actual data object and it gets splattered all over the rendering component. (splattered sounded more dramatic then spread). 2¢

@tleperou

This comment has been minimized.

Copy link

commented Jul 6, 2019

I think I am out of the scope of the RFC which steps on in the simplification of the route's API. @webark

These,

For me, the main advantages of a route, and loading data from the route, is the clear communication it provides

The thing i enjoy about moving from “model” to “load” is that it is more explicit about what is happening, it’s easier to explain to newcomers, and it avoids the awkward feeling when you load multiple items into one model hook, and then need to set aliases in the controller.

sounds utterly true for me too.

For years now, Ember spotlights the component. Such as, Create Router Service, Deprecate Route render APIs and An alternative to Controllers (plus others I missed) which get features off of Route/Controller from Ember.

From my point of view, the "clear communication [Ember] provides" through the different application's layers is essential. The route makes the app a Web application. Indeed, we use an URL.

Considering a route as a URL defined into the router.js which instantiates a Component, would provide the same features to developers with different development experience (no Controller, no Route):

  • (pre/post) process local or remote data: @action, @task do the job
  • Interact with the hooks of the route: the Component's lifecycle and render modifiers fit with
  • Provide a clear communication: keep the file structure
  • Workaround the query parameters
  • handle error and loading states: as we already do in components

Sounds evident there are some caveats but it fits the initial motivation that @michaelrkn, @chadhietala, and @NullVoxPopuli described. It hugely simplifies the APIs and the way to teach.

@NullVoxPopuli

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2019

For the record (maybe as a point of clarification), I did react for 3 years where all you had was components and I'm against replacing routes' model lifecycles with components. :/
Data loading is special enough to be it's own thing.

@Leooo

This comment has been minimized.

Copy link

commented Jul 10, 2019

My opinion is that naming the Ember route loading hook "model" is a very conscious choice to suggest that the default object to be yielded by this hook should in general be a part of the domain model - typically an ember-data model.

A book route should yield a book model to the templates. Just MVC where the routes are the C and should yield (in general) ONE model to the view layer.

@samselikoff is saying it brilliantly in this article. Were this hook to be renamed loading, I would be very worried to see even more devs yielding huge RSVP.hash({}) objects in their routes instead of leveraging the model layer, which is in my opinion strongly a bad pattern.

I know that moving away from controllers yields at first to huge RSVP.hash() in model hooks. But the final stage should be instead to distribute the logic across models & relationships so that one single "natural" model can instead be consumed by the components / templates.

@mehulkar

This comment has been minimized.

Copy link

commented Jul 12, 2019

My first thought at reading of a load hook was also to load specific components required by the route. (i.e. lazy loading, code splitting). I'm assuming this would already be possible with dynamic imports and ember-auto-import, but might be a good idea to mention it in this RFC as well.

@michaelrkn

This comment has been minimized.

Copy link
Author

commented Jul 12, 2019

My first thought at reading of a load hook was also to load specific components required by the route. (i.e. lazy loading, code splitting).

@mehulkar This RFC builds off of #499, which creates an API to load a single component from the route. If it doesn't seem clear in the context of #499, can you comment/PR to help me improve the clarity? Thanks!

@webark

This comment has been minimized.

Copy link

commented Jul 12, 2019

@michaelrkn did you ever toss around just a “data” hook rather then “load”?

@michaelrkn

This comment has been minimized.

Copy link
Author

commented Jul 12, 2019

did you ever toss around just a “data” hook rather then “load”?

@webark Yes, but I wanted to base this off @chadhietala's proposal where he proposes that this hook also take responsibility for redirection, which I think is a good idea. data doesn't convey that redirection could occur here. load also matches with the loading state in #499.

@webark

This comment has been minimized.

Copy link

commented Jul 12, 2019

oh, and @michaelrkn did you ever read over the discussion around #97 ?

It’s a little tangential, but there’s some good discussion around the semantics of loading data in a route.

@michaelrkn

This comment has been minimized.

Copy link
Author

commented Jul 12, 2019

@webark Thanks for pointing that out! Do you think the proposed load hook should have the parallel loading behavior suggested in #97? Are there any other takeaways?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.