Introduce Route Serializers

[trentmwillis]

Rendered.

[nathanhammond]

All of my feedback was addressed prior to this being posted. 😄

@rwjblue @dgeb @stefanpenner and everybody else following along at home we'd love to hear your thoughts.

[raytiley]

This is pretty similar to a discussion @rwjblue and I had a while back.

I think the RouteSeralizer should probably be a full blown object that can serialize the route url as well as any query-params defined for that route. Being able to link to Routes that use query params without needing to load the full route / controller etc is super important.

[trentmwillis]

@raytiley thanks for the input. Whether or not to roll query param support into this is definitely an open question. However, it looks like the only relevant QP info for linking to a route are default values which get removed from URLs, but there has been discussion over whether that behavior is even correct/needed.

Are you aware of any other QP-related behavior that would be needed before attempting to enter a route?

[davewasmer]

Just two cents from one of the "folks at home" 😉 ...

As someone who has worked daily on a decently complex production Ember app for 1.5+ years: I'm not entirely sure I understand the need or use case for this. To be clear, I'm sure it exists, but from a pedagogy perspective, the provided examples look identical to me, except now the serialize function is separate. I get that it will improve the handling of asynchrony in routes, but it's not clear why.

In my experience training the junior devs on my team, a common struggle learning Ember is the number of concepts to grapple with. Now, there's a decent chance I'm just being dense at the moment and the use case is obvious. But if that's not the case, I'd be concerned about introducing a new "top-level" concept. Especially if I can't grok it quickly, it will be even tougher and seem more arbitrary for the less experienced devs on the team.

Like I said above, I'm sure something like this change is necessary. I'd just caution that the reason why it exists should be articulated clearly to even beginners. Certainly the RFC language doesn't need to be written at that level, but just a friendly heads up from the audience.

[raytiley]

but it's not clear why.

@davewasmer imagine you have a BIG app with a user facing section and an admin section. You probably want to link to different admin sections from the main app, but you want to reduce your payload size by not delivering the admin section to the client unless they enter it.

This RFC allows you to link to those sections of the app, without them actually being downloaded and evaluated by the client. It's almost entirely for performance, although it can make dealing with larger code bases more maintainable to separate them.

[raytiley]

@trentmwillis

It's been a long while since I worked on the query params stuff, but basically the Route object as two hooks for serializing / deserializing the query-parsms. I thought they were public, and not sure if they were supposed to be but get turned private in the great private / public change over a few months ago? @machty might know.

The hooks: https://github.com/emberjs/ember.js/blob/76e9c595809957c14d6616d45be35ae2fe94fd3b/packages/ember-routing/lib/system/route.js#L361-L395

The idea for overriding them was that you might want to take a a complex object and control the serialization so you could make it prettier than calling JSON.stringify on it and sticking that in the URL.

[workmanw]

I would ditto what @davewasmer said. Our app has been around for a while. We almost never have to implement serialize because the default implementation will ulimate do get(model, 'id').

We do have a couple of cases where we have "base route definitions" that similar routes extend, eg:

import BaseFileRoute from 'app/routes/base-file';

export default BaseFileRoute.extend({
  /* ... */
});

Alternatively, you could create a reusable mixin. Also, you could just create a reusable pure functions for serialization/deserialization based on type.

So with all of those existing options in place. I personally think this adds complexity without providing much benefit to most users. And advanced users have some options as mentioned.

---

Edit: I did not fully appreciate the async route loading scenarios when I wrote this. So I acknowledge that aspect of this RFC is new functionality. But I still am skeptical of the number of people who would use this.

[stefanpenner]

will improve the handling of asynchrony in routes, but it's not clear why.

I'm not sure what the solution should be (this may be it), but the problem is as follows:

1. many want to be able to lazy load parts of their app
2. many want to link to a portion of the app that is lazy loaded
3. url construction is (do to serialization) a combined effort of link-to, the router and the target route.
4. if the target route does not yet exists, how does one calculate the URL?

It's a bit of chicken + egg problem, one approach is to separate out the part of a route responsible for serialization, so that it can be part part of the eagerly loaded bundle.

In-practice, although some users rely on this feature (it serves a real purpose), many more users are totally unaware of its existence and get by just fine. I would say adding an extra thing sure is unfortunate (and if it can be avoided it should), but in general this thing isn't on that many critical paths...

I did a quick survey of several million lines ember related code on my machine, and this is used 7 times, but those use-cases are legitimate.

What does this mean? Adding this concept (if that is the approach taken) has very small downside (as it affects very few users), and an upside of enabling asynchronous loading of engines.

[stefanpenner]

We almost never have to implement serialize

Then this RFC isn't applicable to you. This RFC exists to explore/solve a real problem blocking async engines, this feature (which you don't use, but is sometimes useful) needs an alternative for us to move forward.

[raytiley]

and if it can be avoided it should

Could it be solved by tooling? Since we are just extracting a function this is currently on the route to the eagerly loaded bundle, couldn't the ember-engines addon do this at build time?

[stefanpenner]

Since we are just extracting a function this is currently on the route to the eagerly loaded bundle, couldn't the ember-engines addon do this at build time?

This is an option, and was discussed earlier, it wasn't ruled out, but some concerns arose let me share:. The transformation is pretty surprising, as basically this one function can only support some limited subset of the language. Enforcing this by creating a different "thing" at-least reduces that risk, at the cost of another thing (some users may want).

[Panman82]

This RFC exists to explore/solve a real problem blocking async engines

I think that needs to be mentioned in the RFC. When I seen Ray's response I was going to reply back "ah, engines?" not realizing this is for async engines.

I agree with others that it seems a bit overkill but understand if it's needed for async engines, yet another type but what do you do...

[mmun]

How does putting it in a separate module help? Why not just ship the entire route.js files themselves? I'm not opposed to this RFC but it should be spelled out.

Don't do this and continue loading and instantiating all route information upfront. This is bad for performance

Is it really?

and keeps concerns coupled.

I agree that that in order to talk about a route, you don't need to know how it wants to load its model. But there is a cost to the decoupling. I don't want to have yet another top-level folder in my project directory. Maybe with pods it will be more ergonomic (routes/post/serializer.js).

[mmun]

I guess there are people who heavily put actions in routes who don't like the idea of shipping all the route.js files. I keep the vast majority of my actions on controllers so my routes end up being lean (just a model hook and redirection logic typically).

Question: If you had the route.js files in advance, could you preload the models while you were loading the async engine? Are we throwing the baby out with the bath water?

[trentmwillis]

How does putting it in a separate module help? Why not just ship the entire route.js files themselves?

A separate module which is just a function means that there is no backing object that would need to be instantiated. This is tremendously beneficial especially in apps where all routes extend from some other base class. It also means any miscellaneous dependencies used by the route won't need to be loaded just for this one function.

I don't want to have yet another top-level folder in my project directory.

I agree this is a strong downside, but as others have mentioned, the majority of projects will never even see this.

If you had the route.js files in advance, could you preload the models while you were loading the async engine? Are we throwing the baby out with the bath water?

Potentially, but going back to my first response what happens when your route depends on a parent class and that class imports utilities from other addons (this occurs in an application I work on)? We'll have to draw a line somewhere. Maybe this isn't the right line, but we would have to establish some criteria for it.

[trentmwillis]

@raytiley if there is any sort of public way to serialize query params then I think we'd definitely want to roll that into this construct.

Could it be solved by tooling?

I imagine so, but aside from @stefanpenner's concern, it would also add more "magic" than I feel should occur. I would definitely think an ember-watson addition should be created to help migration though as I think this is a perfect candidate for it.

[mmun]

this occurs in an application I work on

Mine too (using ember-simple-auth). And for the reason I'm ok with this RFC with the current state of things. I do think the next logical thing will be wanting to load the models in parallel with the engine whenever possible. This is similar in spirit to the prefetch RFC. But perhaps that is asking too much.

[trentmwillis]

This is similar in spirit to the prefetch RFC.

We use prefetch currently as well and would definitely like a solution that accommodates it and async engines, but we haven't been able to think of a solution within the current system.

[stefanpenner]

This is similar in spirit to the prefetch RFC.

seems orthogonal

[courajs]

Once that is done, we can deprecate Route#serialize over the remainder of the 2.x series to give developers the time to update their code base. We can then remove support in 3.x.

Requiring my tiny route serialize function to be in its own file seems like pointless overhead for most apps. By not deprecating here, can we keep the common case simpler? It seems to me the existence of the route serializer module could just override a Route#serialize if it exists.

[trentmwillis]

It seems to me the existence of the route serializer module could just override a Route#serialize if it exists.

This would be fine if the plan was to continue using the entirety of Route for all of routing, but the goal is to "separate knowledge about how to link to a route and how to enter a route".

So instead of overriding Route#serialize with a route-serializer, we would need to do the reverse and extract Route#serialize to use as a route-serializer so that we separate out knowledge about how to link to a route. That would require instantiating the entire route which winds us up back where we started.

[mmun]

@stefanpenner The spirit of prefetch is to load things faster by doing network requests in parallel. The spirit of loading the engine and its models together is to load things faster by doing network requests in parallel. It's literally the same purpose. If we support the former, we should at least explore whether the latter is possible.

Edit: Reworded.

[Panman82]

Instead of yet another file / folder structure, what if the route.js file exports the function.?. It would be clear that function is separate from the actual Route object, and tooling could import just that function. I suppose that would require all route.js files to export a function, or is there a way to handle a missing export??

Edit: I'm assuming @raytiley mention of tooling is about somehow magically extracting the existing Route serialize method/function.

[stefanpenner]

maybe we are talking about the same thing but words are different.

I'm going to describe what I believe is motivating the RFC, just to be sure the motivations of the RFC are correctly understood.

• prefetch is improving the ability (during transitions) to pipeline asynchrony. It does so, by allowing every route, that is likely to be entered to participate early.
• async engines (which this RFC aims to unblock, by allowing one to generate link-to href's for code not yet loaded) enable the on-demand loading of segments of ones application.

Improved concurrency seems like a cool emergent property of async engines, as one can imagine a slim shell that lazy-loads all large slabs of work, but I would say that is an emergent property, rather then a motivating factor. Instead I would say the motivating factors is primarily to reduce up-front load times, by making different aspects of the application pay as you go. So rather then incurrent byte size the penalty of the entire app, one only incurs the penalty of that segment of the application.

[stefanpenner]

Instead of yet another file / folder structure, what if the route.js file exports the function.?. It would be clear that function is separate from the actual Route object, and tooling could import just that function. I suppose that would require all route.js files to export a function, or is there a way to handle a missing export??

some responses:

• It would be rare for users to add this file, as in-practice it seems infrequently used (so people don't need it, wont notice it because it wont be present)
• merging serialize with its route.js, brings us back to non-obvious separation which requires more magic to accomplish. see: Introduce Route Serializers #120 (comment)

[stefanpenner]

people about to comment, please be sure to read the first line as this RFC is providing an proposed alternative to an existing feature, a solution is required to decouple an engine's routes from URL generation.

Without a solution, one cannot link to engines which have not yet been loaded.

Introducing a (rarely used) new entity, although quite unfortunate, is one approach which provides very clear / easily verifiable separation.

[mmun]

@stefanpenner I agree with everything you wrote. 👍 I was discussing something else entirely (if you bundle all the route.js files that the engine defines you can do much more interesting things at the cost of those bytes, like preloading engine routes' models while loading the engine bundle, redirecting, etc. It seems complicated and I don't feel strongly about it so I'll drop it).

I especially agree with the two points #120 (comment). In my experience Route#serialize is very rare. I've seen one among all the apps I've worked on.

[Panman82]

merging serialize with its route.js, brings us back to non-obvious separation which requires more magic to accomplish.

It's still a route related thing...

I suppose we should also talk about making Transforms, and possibly an Adapter layer.

[dgeb]

Since the need for custom serializers is rare (as others have said), most developers can ignore the entire concept until an app actually needs it.

I'm also not in favor of magic extractions of this method from routes. It's better off as a pure function in a standalone module with clear dependencies.

I am 👍 to this RFC. Thanks for writing this up @trentmwillis. It's just what @rwjblue and I had in mind for enabling async engines.

[stefanpenner]

@trentmwillis although not a firm 👍, treating serialize as a stateless function, used as part of router.map was surprisingly well received at todays meeting.

[knownasilya]

This is nice, because you define the dynamic segments in the router, so the serialize makes sense there. If you need a big process, just import a utility..

[trentmwillis]

@stefanpenner @dgeb what should be the next step here? I've submitted a POC to get a feel for potential implementation and it seems mostly straightforward.

Only unresolved question (that I can think of) would be if there is a use-case for needing state in the serialize function, but I haven't seen any and can't think of any.

[dgeb]

@trentmwillis yes, thanks very much for the POC! I'll review your PR soon and move this along.

Only unresolved question (that I can think of) would be if there is a use-case for needing state in the serialize function, but I haven't seen any and can't think of any.

Same. I'm pretty sure there's consensus that state should not be needed in the serialize function. This assumption is critical for lazy loading routable engines, regardless of how engines export their route serializers.

[trentmwillis]

Awesome! Thanks @dgeb.

I'm pretty sure there's consensus that state should not be needed in the serialize function. This assumption is critical for lazy loading routable engines

Makes sense.

[mixonic]

@trentmwillis thanks for the POC, looks really great. I've added this to Friday's agenda

[MiguelMadero]

tl;dr; Why don't we just simply use a different helper when we need to use the new serialize function and still have {{link-to}} with the default behavior for everyone else? Anyway, given that everyone rarely uses serialize, moving to it even without ember-watson it should be trivial. Those not using engines won't need to worry until 3.0, so a big 👍 to this RFC.

I get the importance of this for async. I had to solve this in a couple of projects where we were lazy loading packages. While having a separate Route#serialize would've helped us, we worked around it just fine in two ways:

1. Avoid using the link-to helper for routes that might not be available. Instead, we simply added a link with an href="#routeName/param1", not great but it's simple and did the trick for 99% of the cases we had links across packages. The problems is that it assumes not only the serialization logic but also the path to a route, additionally, it would call the model hook since we don't pass a model, in most cases that's not an issue since we can rely on caching to avoid that op from being expensive, but it's something to consider.

2. Have a new helper to replace link-to that uses the routingService so it can transition, check the active class, etc, but relies on a default implementation of serialize.

   {{link-to-external-package 'routeName' model}}
   

This way, everyone can keep using the current Route#serialize in the rare cases when it's needed and only those using engines pay the cost when they have links across packages/engines.

The drawback of this is that we're creating some coupling now because the consumer needs to be aware of whether the route is fine with the default implementation for serialize, but most of the time the default implementation works just fine based on Stefan's sample of millions of line of JS code and a quick grep I did on a few large projects, that's rarely even used.

An additional workaround, that I never used, but thought of it for cases where we wanted to override serialize was to rely on a loose function that we can pass to the link-to-external-package helper.

{{link-to-external-package 'routeName' model serializer=routeNameSerializer}}

Then routeNameSerializer is simply a prop in your component to a pure function that you can import.

// components/consumer.js
import routeNameSerializer from 'shared/utils/route-name-serializer'

export default Ember.Component.extend({
  routeNameSerializer: routeNameSerializer
});

The problem with that is that now your component needs to know about everything that is being linked by its template and we still need to know of cases where we actually override the serializer, which ideally the component should be unaware of. This can be solved by simply relying on the serialze function proposed on this RFC defined as part of Router.map, but still using a new helper:

{{link-to-external-package 'routeName' model}}

[dgeb]

The core team discussed the pedagogy and strongly favors separating the definition of the serialize function from the main route map. This is analogous to how we separate the declaration of initialize functions in initializers.

@trentmwillis could you please change the example above to something like:

// app/router.js

function serializePostRoute(model) {
  // this will make the URL `/posts/12`
  return { post_id: model.id };
}

export default Router.map(function() {
  this.route('post', { path: '/post/:id', serialize: serializePostRoute });
});

[trentmwillis]

Sounds good. Will update this and also update the proof-of-concept PR to reflect this pattern as well.

[dgeb]

tl;dr; Why don't we just simply use a different helper when we need to use the new serialize function and still have {{link-to}} with the default behavior for everyone else? Anyway, given that everyone rarely uses serialize, moving to it even without ember-watson it should be trivial. Those not using engines won't need to worry until 3.0, so a big 👍 to this RFC.

@MiguelMadero if Route#serialize is so rarely used, there should be a very low cost to deprecating it and moving it to the route map as proposed here. I'm more concerned about the cost of remembering to use a special helper instead of {{link-to}}.

[MiguelMadero]

Agreed!

On Wed, Mar 16, 2016 at 1:10 PM Dan Gebhardt [email protected]
wrote:

tl;dr; Why don't we just simply use a different helper when we need to use
the new serialize function and still have {{link-to}} with the default
behavior for everyone else? Anyway, given that everyone rarely uses
serialize, moving to it even without ember-watson it should be trivial.
Those not using engines won't need to worry until 3.0, so a big [image:
👍] to this RFC.

@MiguelMadero https://github.com/MiguelMadero if Route#serialize is so
rarely used, there should be a very low cost to deprecating it and moving
it to the route map as proposed here. I'm more concerned about the cost of
remembering to use a special helper instead of {{link-to}}.

—
You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#120 (comment)

[trentmwillis]

Updated the examples given to reflect separating the serialize function declaration and passing it into the router map.

[dgeb]

@trentmwillis thanks for updating. I think that was the only blocker to merging this RFC as well as your POC. I'll try to keep this moving this along ...

[rwjblue]

I agree RE: landing the RFC, but I think the POC needs a bit of work (some compat tests, deprecations, etc), though it is possible that has been added since I last reviewed it...

[trentmwillis]

@rwjblue I've added a deprecation and tests for the cases I could think of. Just let me know of anything else that is needed.

[mixonic]

Hey folks, @trentmwillis. At the last meeting I brought this up, but I didn't follow through here. Sorry :-/ I'll publish the relevant notes and link.

Updating the examples to move the serialization declaration out of that map was indeed the last blocker on merging this RFC. It would have been nice if "Pedagogy" was also updated referencing that change, as that style is related to how we present this API to end-users. I'll make a followup commit to address that.

Mergin'! Thanks for your thoughtfulness and effort as we work toward consensus all 🎉

[grapho]

Sorry to be late to the party.. as this is already merged i just wondered if anyone would know if this use case was considered or not?

model(params) {
  return RSVP.hash({
    person: this.store.findRecord('person', params.person_id),
    miscData:  this.store.query('misc-data', { filter: params.person_id })
  });
},

serialize(model) {
  return { person_id: model.person.id };
}

In this case, it can easily be extrapolated by looking just at the route file, why some extra serialization is needed to perform. When the serialization is moved over to the router.js, it will not be clear why a serialized id must exist at model.person.id.. one would need to examine both the router and the route in question before considering any refactor.

So, since this is already merged and seems to be the agreed path forward, I will ask.

1. is my above example uncommon, or anti-pattern?
2. is there a nicer way to do what i am doing, with the new proposed route serialization?

[knownasilya]

I think it's a non-issue because how you get the id is really the same. How many levels you have to go down will just depend on the route.

This refactor in general is a loss in clarity, since the serializer is related to the model and the router has no visual context to that, but it was either this, or have it in it's own file, which is even less clear, because you don't get the path: ':person_id' context, that you get from the router.