[Route Prefetching] Allow routes to request data in parallel #97
Conversation
|
👍 - I love the goal and the general idea.
The API's that are designed in this RFC need to be completely backwards compatible or this change has to wait until Ember 3.0 (which is quite a long way off). FWIW, I believe that it is possible to make this migration in a backwards compatible way with enough thought and planning... |
|
👍 What would happen if a transition is done in a |
|
|
||
| # Drawbacks | ||
|
|
||
| - This is a major breaking change. |
There was a problem hiding this comment.
I think a way of getting around the incompatibilities would be to create a new object type, that opts you into new semantics. There is a precedents with this type of feature introduction with GlimmerComponent. This could be something like ParallelizedRoute, NonblockingRoute, MyAPIIsSlowSoDoMoreThingsInParallelRoute, etc.
There was a problem hiding this comment.
There is a precedents with this type of feature introduction with GlimmerComponent.
Agreed. Definitely a possibility, but we (mostly @wycats, @tomdale, and @chancancode TBH) worked very hard to not need to make a different base class and discovered through that pain that it was unavoidable. It was definitely a last resort...
There was a problem hiding this comment.
@rwjblue yup.
I think a separate name, and provided via community add-on (with minimal hooks in internals) may be an escape valve. I want us to be careful not to cram ideas (good or bad) too quickly into ember. As once they are added, it is tricky and extremely costly to unwind.
We can look to several hasty (although well thought on 1.13.x features) as a demonstration of this.
I do believe the motivation here is good (and demonstrates real business value), the implementation may also. But it is well known the more users we can get using this in the wild the faster we can explore the problem space, and the more unknown unknowns we can squash before it becomes part of the finalized public api.
There was a problem hiding this comment.
To help explore the problem space I'd love to get an early version in behind a feature flag so we have fewer unknowns. It seems like it would be a good fit to come up with an API for this sort of behavior at the same time as trying to solidify routable components so that they play nicely together. It also gives us a GlimmerComponent/"use strict"; type of escape valve for opting in if we can't come up with a better way to handle it.
The option of building an addon has community effects that will be quite painful. We see tight coupling between liquid-fire and Ember that an addon to accomplish this would make look positively decoupled. Self-trolling will occur all over the place.
|
I'm good with the idea of running many routes in parallel. But I'm not ok with running all the hooks within a single route in parallel. If you want your model hook to start immediately, don't return a promise from your beforeModel hook. FWIW, there's already a proposal in the routable components RFC to unify all three hooks and require |
|
There is also another approach, all hooks stay the same. But additional hooks (or ideally additional pre-fetch objects) are added purely for eager loading / prefetching. The aim is no to disrupt the existing programming model, but provide the same latency reduction this proposal aims to solve. Something like: export default Ember.Route({
async prefetch(params) {
// executed upfront, allowing each route to opt into pre-fetching semantics.
// this should be thought of as nearly entirely declarative
// one could imagine also providing graphQL esq (or actually) declarations, that can be composed at runtime.
return {
user: this.store.find('user', await this.prefetched('parent.route').user.id),
}
},
model(params) {
// .. executed normally, after the route is entered and the routes own prefetch has completed
// relying on identify maps and such which have pre-fetched.
return this.store.find('user', params.user_id)
}
]);This makes prefetching:
The algorithm would be as follows:
This should:
|
|
@stefanpenner I think that whole idea can be done in userspace using public APIs once we ship the changes proposed in #95. I think that's a good solution for teams that are itching for a solution right away. We would still want to iterate to a fully baked-in solution that will Just Work. |
|
@ef4 I agree with you that Can you give an example of using the API from #95? I don't follow. @stefanpenner Interesting idea to add an additional hook for eager loading. Would it rely on Ember Data caching to get the prefetched data in the |
|
The prefetch result could be made available in the transition object passed to the model hook, but it feels like a lot of hoops to jump through. I think |
This is the recommendation that we've given for a long time even with In my opinion the thing that distinguishes As more of a power user I'm of course in favor of running all the things in parallel, but I recognize that the consequences of that for the majority of the Ember ecosystem are nontrivial. Unification under routable components neatly solves this problem. |
|
@stefanpenner The API you propose is interesting, but also assumes that the only async behavior we're dealing with is loading of data. It could also be writing to disk (NW.js) or a long-running process that we handed off to a WebWorker. Granted, those are less likely and we should aim for making sure that the happy path is optimized. Even in the world where we accounted for all use cases it would still likely make sense to have a For example, @nickiaconis implemented multiplexing on top of Ember Data already which is one of the primary motivations for this RFC: collating across route boundaries. Discussions are being had about how to do client-side composition of data models in the vein of Falcor/GraphQL such that it is a fully-resolved data-dependency tree instead of still requiring round trips between tree "layers" (depth from root node(s)). |
Either a identity map, or other hooks (like
This seems orthogonal.
the prefetch recommendation doesn't hinder this.
the prefetch recommendation doesn't hinder this. |
|
@stefanpenner We could turn the whole prefetch hook into a DSL which we can serialize into Ember Data calls, lisp-like functional transformations that could be passed to the server a la GraphQL, or into vanilla Ajax calls. We just design/adopt a way to describe what data you need as a fully resolved tree and then let some serialization of that definition inform your |
|
@nathanhammond yes I believe My motivation is to find some stepping stones which bridge the gap between declarative/prefetch ideas and the existing world. I believe the existing world has many merits and strengths, we would be wise not to fall into the trap of believing a drastic departure is required. The problem as I see it takes the following shape:
It seems like both of these can complement each other nicely. In-fact it should be possible to minimize changes to the existing, while still reaping the rewards of a globally optimized system. Some thoughts:
Taking the layered approach may prove some existing API / timings redundant, which means in 3.0 they can be removed. Drastic departure, of making everything concurrent feels quite hostile to existing applications, that may or may not benefit from this. In retro-spec the existing API's are fairly zalgo and would likely take different forms if implemented today. |
@nathanhammond This is almost precisely what I'm working on in Orbit.js, which will soon have a lisp-like customizable query expression language. More details here: orbitjs/orbit#212 This approach allows for complex, customizable, and synchronous queries of in-memory data, which can be asynchronously "hydrated" from remote sources. When used with Ember, this will allow many model hooks to be resolved synchronously while still asynchronously fetching data. The actual fetch calls could be batched, as you suggest, depending on the capability of remote sources (GraphQL, JSON API, etc). Of course, there are still going to be remote calls that should block route loading, in which case a promise should be returned to the model hook. But for many cases, synchronous resolution of in-memory queries combined with asynchronous fetching of remote data will provide the best user experience. |
|
Here's what I think we should do:
Once these behind-a-flag APIs are in place, we can get more real-world feedback on what patterns (other than the obvious ones) emerge, which we can feed into the design process for a new public API. For what it's worth, I am very confident that the public version of this API requires |
|
Came here to chime in that I think different routes should load in parallel but am opposed to @wycats I am a little nervous about putting the functionality in behind a flag if we know that is not the final, desired API. Mostly because people will likely come to rely on it, and it may be a large refactor to go from e.g. a flag on existing routes to an entirely new |
I think we need to learn more from users who get real benefits from parallelism before we can design the API. Today, users in that boat are monkey-patching into private APIs, which I think you can agree have a worse version of the problem you're worried about than a few known flagged APIs. |
|
TLDR, in TC39 terms: let's move this proposal to Stage 0 😉 |
Agreed.
When folks monkey patch private API's they are clearly taking on any future issues themselves. This is not true of feature flags that have landed in core (where folks report and expect bugs to be fixed by the magic OSS fairy).
Agreed, making an addon that provides the strawman proposal sounds wonderful! |
There have been several suggested alternatives to the original proposal. What are we moving forward with? I can update the RFC to reflect what it is we want to be implemented as Stage 0. |
I'm not sure how to get our target audience (mid-size applications, moderately familiar Ember devs) to test this since everybody commenting on this thread has large apps and already knows the internals of the router. We're all terrible as case studies.
Our tradeoff:
I feel like the second one has less of a risk of becoming the next vendor prefix debacle, but that's just me. |
I don't feel like this is what happens. When an addon that uses private APIs becomes popular, it becomes just as much an entitlement as an opt-in flag, but more so. |
I respectfully disagree. |
Liquid Fire users have not felt like they were to blame for using private APIs, while users of the unstable |
|
This is a great example, |
|
Ping.
|
|
@stefanpenner -- The more I think about it, the more I like your idea of a Also, to get a feel for it, I've taken a stab at implementing prefetch as an addon. It's almost working, but I've run into an issue. |
|
|
||
| # Drawbacks | ||
|
|
||
| - Ember's API becomes larger. |
There was a problem hiding this comment.
it may be confusing to the programming model
|
Im not quite sure (from reading this) what happens on refresh of a route, like when a QP changes and the model hook is intended to be run. Or what happens on a link-to (with model, not id provided) pivoting on the route which has a prefetch. |
|
|
||
| async model() { | ||
| return { | ||
| OP: this.modelFor('post')).author, |
There was a problem hiding this comment.
Need linting for code blocks in markdown. 😛
|
@nickiaconis We discussed this in the core team meeting today and we had some concerns with the RFC as is. @stefanpenner wanted to make sure this didn't languish, so he asked me to write the concerns down. I speak only for myself but have tried to incorporate the problems other core team members identified. I'll start by laying the philosophical groundwork then get down to the merits of this specific RFC after. For me, the high-level concern is this: as software libraries grow, there's a tendency to add additional API as new use cases (and deficiencies in the original design) are discovered. It is very tempting to keep merging small, targeted new APIs to solve pain points that developers are facing in the real world. This is great for advanced users; learning a series of small changes is easy because it's amortized over months or years. For new users, though, it make the framework feel like an incoherent soup of complexity. They have a hard time creating a strong mental model and they may abandon the framework altogether. Angular 1 is a good example of this: I see new users frequently confused by services, factories and providers. They solve similar problems and you have to understand their subtleties to know which one to use. And directives have so many knobs to turn (transclusion, scope, The way to fix this is to periodically gather up all of the constraints, including the new ones discovered since the original feature was developed, and see if you can find a new Grand Unified Feature that solves all of the constraints with one concept instead of several. Many of us on the core team think that it is probably time for that to happen for model loading in routes. Rather than adding a parallel For example, perhaps we replace the // app/routes/post/comments.js
export Route.extend({
fetchModel() {
return this.waitForModel('post').then(post => {
return $.getJSON(`/posts/${post.get('id')}/comments.json`);
});
});This proposal almost surely has flaws that need to be ironed out, but it's an example of what I mean: rather than having a serial model pipeline and a parallel model pipeline, we try to find one pipeline that can do the right thing based on what the developer expresses inside. I think the best way to move forward is to create a document that enumerates use cases from the real world. Offhand, that includes at least a model that depends on a parent, two models that don't have any dependencies, and a GraphQL example. Once we have a list of the use cases, we can see how the proposed API looks in context. As it is, though, I think the current RFC has way too little detail and consideration of all use cases for something that touches such a fundamental part of the programming model. |
@tomdale If on every new discovery we have to re-invent the world, we end up with a different but also high cost learning hazard, as the paradigm shifts (rather then simply evolving it). As what happens (as evident in prior such re-works) the grass isn't actually any greener on the other side, instead a new fancy paradigm appears, shifting the problem space rather then reducing it. We should first expand (carefully), before re-inventing the whole model. I do not believe supporting this in core requires a rework of the entire programming model, in-fact with some more massaging of what is proposed here complements the existing model nicely. My suggestion is that we massage this RFC, ensure the learnability/understandability address @tomdale concerns as best as possible, then let it ride the release trains, get feedback. A year from now, when we have time/energy/motivation to improve the routing DSL we can use additional knowledge it provides to make better and more informed choices in a redesign (if that is even required). It seems like the mood is surprisingly negative, if that is the case. Maybe we should explore codifying the exact required missing events/API hooks to implement this well in user-space and call it a day. This would be unfortunate. |
|
I'm a bit surprised at the degree to which you have concerns, @tomdale. We have a working implementation of this in production for LinkedIn, a popular addon implementing this RFC, and a pretty compelling demo. In addition, internally, @nickiaconis built a dynamically constructed request multiplexer on top of this simple Philosophically I disagree with you as I'm concerned that a 2 => 3 transition where we move to something like On the topic of "stability without stagnation," a non-blocking model hook has been a feature that people have been asking for since day one. So far the answer has been "just return a plain object and set properties on it later" which is pretty clearly an inferior version of this solution. Not providing a primitive for this behavior (which has been desired since before 1.0!) until the 3.0 timeline feels like a disservice to users of Ember. I truly feel like we should address this now. Regardless, @tomdale, do you hold the same reservations about adding the underlying primitives necessary to build this in user space? Something like |
|
FWIW I'd like to see what a hook supporting the addon and use-case would look like. If it really is trivial, it could go straight to a PR and feature flag and likely be discussed/merged from that. Small win, I know, but potentially a win. |
|
@nathanhammond Trying to amp up the sense of urgency is not going to be a winning strategy. The pain is real and I want to fix it. I am happy that you have a proof-of-concept that is working well for you. However, I've been doing this long enough now that I know that rushing in features to solve pain (no matter how acute) ends up being worse in the end, even if I can't yet guess how. As @mixonic identified, the best path going forward is to figure out the minimum set of hooks we need to expose via public API to let an addon implement this in a forward-compatible way. That will help us eliminate the pain as rapidly possible while buying us time to have a thoughtful discussion about the programming model. |
Maybe this is semantics, but I feel like my proposal is much more of an evolution of the current model than the |
|
To be clear, the transition strategy for my proposal is:
One thing I'm unclear on is if my proposal works well with GraphQL, which I think is going to end up being very important. That touches on a whole bunch of stuff like when promises are resolved, how loading routes work, etc. All of those things, by the way, are punted on by the So I agree with @stefanpenner that rushing things in always leads us to realize later that the grass isn't greener on the other side. That applies just as much to |
I am surprised that you think this is semantically identical to model. The current flow is as follows first time we discover that this route will likely be required
on each entering of a route
finally on exit of the route.
To this prefetch adds the notion of "we will likely activate this route, you now have the opportunity to preemptively prepare yourselves" which enables pipelining, or other eager work. In a perfect world, Unfortunately, that would be a drastic departure from the current model (as we discussed) so an alternative approach that wasn't as dramatic was requested, this is that approach. I am surprised that now you are saying, we should instead take a more dramatic approach and re-work the model. Should we instead propose that model? Such a model change will obvious cause churn, which is what I thought we were aiming to avoid... |
That is what @tomdale proposed I believe. |
In a meeting, we actually (when the PR was first introduced) decided it was to large a departure, and something less drastic would be required. |
I'm at a loss for what is desired considering this already exists and was (rightly, I think) met with opposition: emberjs/ember.js#12415 I feel like this may, in large part, be an issue of naming.
This is the same transition strategy for the proposed |
|
@nickiaconis ya unsure.. Regardless, It is likely a good idea to get the required router/transition events/hooks in, this will aide your add-on and also aide this if it can be moved forward. |
|
One aside, from random conversation: implicit |
|
@nathanhammond yes. Cancellation may also help (at some point). Due to the apprehension towards this... I believe may the best bet is to push the public API's needed to implement this safely as an add-on. Regardless those API's would be a big win, and if this does land in the future it would be aided by those hooks.. |
|
@stefanpenner That works for me. Do we need a separate RFC to suggest those public APIs or can we (I?) get started with an implementation? |
|
@nickiaconis can you pull together a PR for /cc @mixonic |
|
I believe superseded by #126 |
|
@stefanpenner I fail to see how #126 cover this functionality. |
|
late to the party, what is the general consensus of using prefetch in a new app (ember-prefetch)? one of my coworkers implemented it but then it broke the app because we have some control-flow logic that redirects around our auth wall in our So I'm wondering if it's a good idea to use the prefetch package right now or wait until something comes that is part of ember core, which other addons will recognize? |
Updated to @stefanpenner's prefetch proposal:
Rendered.
The demo is especially insightful (and has been updated to match the new RFC).
http://nickiaconis.github.io/ember-parallel-model-demo/