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

Introduce Route Serializers #120

Merged
merged 4 commits into from Mar 18, 2016

Conversation

Projects
None yet
@trentmwillis
Member

trentmwillis commented Feb 11, 2016

Rendered.

@trentmwillis trentmwillis changed the title from Route Serializers to Introduce Route Serializers Feb 11, 2016

@nathanhammond

This comment has been minimized.

Show comment
Hide comment
@nathanhammond

nathanhammond Feb 11, 2016

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.

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 comment has been minimized.

Show comment
Hide comment
@raytiley

raytiley Feb 12, 2016

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.

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

This comment has been minimized.

Show comment
Hide comment
@trentmwillis

trentmwillis Feb 12, 2016

Member

@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?

Member

trentmwillis commented Feb 12, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@davewasmer

davewasmer Feb 12, 2016

Contributor

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.

Contributor

davewasmer commented Feb 12, 2016

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

This comment has been minimized.

Show comment
Hide comment
@raytiley

raytiley Feb 12, 2016

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.

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

This comment has been minimized.

Show comment
Hide comment
@raytiley

raytiley Feb 12, 2016

@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.

@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

This comment has been minimized.

Show comment
Hide comment
@workmanw

workmanw Feb 12, 2016

Contributor

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.

Contributor

workmanw commented Feb 12, 2016

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

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Feb 12, 2016

Member

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.

Member

stefanpenner commented Feb 12, 2016

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

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Feb 12, 2016

Member

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.

Member

stefanpenner commented Feb 12, 2016

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

This comment has been minimized.

Show comment
Hide comment
@raytiley

raytiley Feb 12, 2016

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?

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

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Feb 12, 2016

Member

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).

Member

stefanpenner commented Feb 12, 2016

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).

@Panman8201

This comment has been minimized.

Show comment
Hide comment
@Panman8201

Panman8201 Feb 12, 2016

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...

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

This comment has been minimized.

Show comment
Hide comment
@mmun

mmun Feb 12, 2016

Member

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).

Member

mmun commented Feb 12, 2016

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

This comment has been minimized.

Show comment
Hide comment
@mmun

mmun Feb 12, 2016

Member

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?

Member

mmun commented Feb 12, 2016

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

This comment has been minimized.

Show comment
Hide comment
@trentmwillis

trentmwillis Feb 12, 2016

Member

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.

Member

trentmwillis commented Feb 12, 2016

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

This comment has been minimized.

Show comment
Hide comment
@trentmwillis

trentmwillis Feb 12, 2016

Member

@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.

Member

trentmwillis commented Feb 12, 2016

@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 comment has been minimized.

Show comment
Hide comment
@mmun

mmun Feb 12, 2016

Member

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.

Member

mmun commented Feb 12, 2016

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 comment has been minimized.

Show comment
Hide comment
@trentmwillis

trentmwillis Feb 12, 2016

Member

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.

Member

trentmwillis commented Feb 12, 2016

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 comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Feb 12, 2016

Member

This is similar in spirit to the prefetch RFC.

seems orthogonal

Member

stefanpenner commented Feb 12, 2016

This is similar in spirit to the prefetch RFC.

seems orthogonal

@courajs

This comment has been minimized.

Show comment
Hide comment
@courajs

courajs Feb 12, 2016

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.

courajs commented Feb 12, 2016

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

This comment has been minimized.

Show comment
Hide comment
@trentmwillis

trentmwillis Feb 12, 2016

Member

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.

Member

trentmwillis commented Feb 12, 2016

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

This comment has been minimized.

Show comment
Hide comment
@mmun

mmun Feb 12, 2016

Member

@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.

Member

mmun commented Feb 12, 2016

@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.

@Panman8201

This comment has been minimized.

Show comment
Hide comment
@Panman8201

Panman8201 Feb 12, 2016

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.

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

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Feb 12, 2016

Member

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.

Member

stefanpenner commented Feb 12, 2016

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

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Feb 12, 2016

Member

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: #120 (comment)
Member

stefanpenner commented Feb 12, 2016

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: #120 (comment)
@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Feb 12, 2016

Member

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.

Member

stefanpenner commented Feb 12, 2016

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

This comment has been minimized.

Show comment
Hide comment
@mmun

mmun Feb 12, 2016

Member

@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.

Member

mmun commented Feb 12, 2016

@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.

@Panman8201

This comment has been minimized.

Show comment
Hide comment
@Panman8201

Panman8201 Feb 12, 2016

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. :trollface:

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. :trollface:

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Feb 12, 2016

Member

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.

Member

dgeb commented Feb 12, 2016

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

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Feb 12, 2016

Member

(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).

Please note, engines have imports + injections etc, they may end up easily retaining large chunks of the engine. Which I believe to be some we should avoid as 👣 🔫's, instead clear cut easily static analyzable engines

Member

stefanpenner commented Feb 12, 2016

(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).

Please note, engines have imports + injections etc, they may end up easily retaining large chunks of the engine. Which I believe to be some we should avoid as 👣 🔫's, instead clear cut easily static analyzable engines

@nathanhammond

This comment has been minimized.

Show comment
Hide comment
@nathanhammond

nathanhammond Feb 12, 2016

Regarding prefetch

@mmun: @trentmwillis and I completely agree that this is an incredibly interesting use case. We had a back-channel conversation about how it could be possible to make prefetch work with this idea of route-serializers. Trent's premise was "fastboot the engine" but in approximately one minute we came up with way too many possible side effects to make that sort of thing work out. You'd have to manage a complete serialized state, ship that to the server, have the server understand that, and then have it emit some bootstrapping file in addition to the engine package, and who knows what else we didn't account for.

Here's the simplest failure demo which would prevent a "fastbooted engine" approach of offloading that to the server:

Ember.Route.extend({
  redirect() { 
    if (this.get('someService.prop')) {
      this.transitionTo('game.over');
    }
  }
});

Static analysis to identify it is is possible to do this/how much we would import could easily become one of @stefanpenner's foot shotguns. The advantage of an engine as proposed is that it is an opt-in to a single-edge, single vertex dependency graph. It's okay for that to have tradeoffs, and we can identify other optimizations to this story as emergent properties, likely in user space.

Also, having spent some time looking into it, anything we do that breaks down the engine firewall is likely better implemented as something that falls out of linker/packager work instead of being accounted for as part of the engines world. @stefanpenner is right that we could accidentally end up unraveling most of the engine by the time we grabbed all of the imports for the now-included routes.

Magic Extractions

All it takes is one this in Route#serialize for game over. If included in route.js as a separate import and extracted then there are still many other things that could possibly be in scope which aren't required for the serializer. Strongly opposed as I see this as a foot shotgun. I'm with @trentmwillis that we can just ember-watson people to move out of the existing world.

Regarding prefetch

@mmun: @trentmwillis and I completely agree that this is an incredibly interesting use case. We had a back-channel conversation about how it could be possible to make prefetch work with this idea of route-serializers. Trent's premise was "fastboot the engine" but in approximately one minute we came up with way too many possible side effects to make that sort of thing work out. You'd have to manage a complete serialized state, ship that to the server, have the server understand that, and then have it emit some bootstrapping file in addition to the engine package, and who knows what else we didn't account for.

Here's the simplest failure demo which would prevent a "fastbooted engine" approach of offloading that to the server:

Ember.Route.extend({
  redirect() { 
    if (this.get('someService.prop')) {
      this.transitionTo('game.over');
    }
  }
});

Static analysis to identify it is is possible to do this/how much we would import could easily become one of @stefanpenner's foot shotguns. The advantage of an engine as proposed is that it is an opt-in to a single-edge, single vertex dependency graph. It's okay for that to have tradeoffs, and we can identify other optimizations to this story as emergent properties, likely in user space.

Also, having spent some time looking into it, anything we do that breaks down the engine firewall is likely better implemented as something that falls out of linker/packager work instead of being accounted for as part of the engines world. @stefanpenner is right that we could accidentally end up unraveling most of the engine by the time we grabbed all of the imports for the now-included routes.

Magic Extractions

All it takes is one this in Route#serialize for game over. If included in route.js as a separate import and extracted then there are still many other things that could possibly be in scope which aren't required for the serializer. Strongly opposed as I see this as a foot shotgun. I'm with @trentmwillis that we can just ember-watson people to move out of the existing world.

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Feb 12, 2016

Member

@nathanhammond yes a this, or any free variable. Meaning, it may live physically within a routes module, but it cannot actually interact with the module. (Not without sufficient static analysis).

Which, I mean isn't off the table. But has sufficiently more unknown unknowns to make me nervous.

Member

stefanpenner commented Feb 12, 2016

@nathanhammond yes a this, or any free variable. Meaning, it may live physically within a routes module, but it cannot actually interact with the module. (Not without sufficient static analysis).

Which, I mean isn't off the table. But has sufficiently more unknown unknowns to make me nervous.

@webark

This comment has been minimized.

Show comment
Hide comment
@webark

webark Feb 12, 2016

Just an idea, but since this is so rare, and it's just a function, could it be added into the routes definition? It's a little ugly, but due to its rarity, that might not be a bad thing, and the route definitions I assume would already be extracted out and included globally, plus, it wouldn't add another thing.

webark commented Feb 12, 2016

Just an idea, but since this is so rare, and it's just a function, could it be added into the routes definition? It's a little ugly, but due to its rarity, that might not be a bad thing, and the route definitions I assume would already be extracted out and included globally, plus, it wouldn't add another thing.

@webark

This comment has been minimized.

Show comment
Hide comment
@webark

webark Feb 12, 2016

Something like..

this.route('post', {
  path: '/post/:post_id',
  serialize: function(model) {
    // this will make the URL `/posts/12`
    return { 
      post_id: model.id
    };
  },
}, function() {
  this.route('comments');
});

I think is how it could/would work.

webark commented Feb 12, 2016

Something like..

this.route('post', {
  path: '/post/:post_id',
  serialize: function(model) {
    // this will make the URL `/posts/12`
    return { 
      post_id: model.id
    };
  },
}, function() {
  this.route('comments');
});

I think is how it could/would work.

@trentmwillis

This comment has been minimized.

Show comment
Hide comment
@trentmwillis

trentmwillis Feb 12, 2016

Member

@webark I think I like that idea. It has the benefits you mentioned, plus it also keeps the information for linking to a route in a single location. I also believe we could ditch the routeSerializer method...

@rwjblue or @dgeb thoughts?

Member

trentmwillis commented Feb 12, 2016

@webark I think I like that idea. It has the benefits you mentioned, plus it also keeps the information for linking to a route in a single location. I also believe we could ditch the routeSerializer method...

@rwjblue or @dgeb thoughts?

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Feb 12, 2016

Member

You know, I like the simplicity of @webark's idea as well. It means that an engine's routes.js would contain all that's needed to form links into the engine.

The aesthetics could even be improved by defining the actual serialize function above the route map and dropping it into the route's options (as opposed to defining it inline).

👍

Member

dgeb commented Feb 12, 2016

You know, I like the simplicity of @webark's idea as well. It means that an engine's routes.js would contain all that's needed to form links into the engine.

The aesthetics could even be improved by defining the actual serialize function above the route map and dropping it into the route's options (as opposed to defining it inline).

👍

@trentmwillis

This comment has been minimized.

Show comment
Hide comment
@trentmwillis

trentmwillis Feb 12, 2016

Member

The aesthetics could even be improved by defining the actual serialize function above the route map and dropping it into the route's options

I was thinking the same thing. This would also make potential reuse easier than standalone modules.

Member

trentmwillis commented Feb 12, 2016

The aesthetics could even be improved by defining the actual serialize function above the route map and dropping it into the route's options

I was thinking the same thing. This would also make potential reuse easier than standalone modules.

@webark

This comment has been minimized.

Show comment
Hide comment
@webark

webark Feb 12, 2016

oh totally.. Or imported if you needed to.. I just did it online for explicitness. And glad I could help! :)

webark commented Feb 12, 2016

oh totally.. Or imported if you needed to.. I just did it online for explicitness. And glad I could help! :)

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Feb 12, 2016

Member

I'm dubious of this being part of the route DSL

Member

stefanpenner commented Feb 12, 2016

I'm dubious of this being part of the route DSL

@trentmwillis

This comment has been minimized.

Show comment
Hide comment
@trentmwillis

trentmwillis Feb 12, 2016

Member

I'm dubious of this being part of the route DSL

@stefanpenner care to elaborate? While a bit more verbose than other aspects of it, I feel it makes sense given it describes how to construct portions of a route's url.

Member

trentmwillis commented Feb 12, 2016

I'm dubious of this being part of the route DSL

@stefanpenner care to elaborate? While a bit more verbose than other aspects of it, I feel it makes sense given it describes how to construct portions of a route's url.

@webark

This comment has been minimized.

Show comment
Hide comment
@webark

webark Feb 12, 2016

I would say it's fair to be dubious. But the the choice was made to have the routes be defined as methods rather then just a json blob, which allows for these types of choices to be even considered. And even though it is diverging away from the standard route DSL, it has already been mentioned that it is a very rare occurrence where this would need to be actually overwritten. It also bodes well for not having to do directory crawls for these files, and not introducing a concept that is unlike any of the others. All things currently are first and foremost objects (at least everything I can think of off the top of my head). Having a new type, albeit a seldom used feature, would cause typical roadblocks and misconceptions. You could not include a mixin, it wouldn't be something you would extend, inheritance would look different.

webark commented Feb 12, 2016

I would say it's fair to be dubious. But the the choice was made to have the routes be defined as methods rather then just a json blob, which allows for these types of choices to be even considered. And even though it is diverging away from the standard route DSL, it has already been mentioned that it is a very rare occurrence where this would need to be actually overwritten. It also bodes well for not having to do directory crawls for these files, and not introducing a concept that is unlike any of the others. All things currently are first and foremost objects (at least everything I can think of off the top of my head). Having a new type, albeit a seldom used feature, would cause typical roadblocks and misconceptions. You could not include a mixin, it wouldn't be something you would extend, inheritance would look different.

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Feb 12, 2016

Member

literally just feels bad, I will have to think about it more.

Member

stefanpenner commented Feb 12, 2016

literally just feels bad, I will have to think about it more.

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Feb 12, 2016

Member

I think it is totally related to path, but a really ugly relative :P

Member

stefanpenner commented Feb 12, 2016

I think it is totally related to path, but a really ugly relative :P

@trentmwillis

This comment has been minimized.

Show comment
Hide comment
@trentmwillis

trentmwillis Feb 12, 2016

Member

Updated the RFC to reflect @webark's approach and provide more clarity around motivation (enabling async engines). Still want to encourage discussion around it, but I think that the new approach meets our goals while addressing the concerns of introducing a totally new construct into the programming model.

Member

trentmwillis commented Feb 12, 2016

Updated the RFC to reflect @webark's approach and provide more clarity around motivation (enabling async engines). Still want to encourage discussion around it, but I think that the new approach meets our goals while addressing the concerns of introducing a totally new construct into the programming model.

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Feb 12, 2016

Member

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

Member

stefanpenner commented Feb 12, 2016

@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 comment has been minimized.

Show comment
Hide comment
@knownasilya

knownasilya Feb 13, 2016

Contributor

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..

Contributor

knownasilya commented Feb 13, 2016

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

This comment has been minimized.

Show comment
Hide comment
@trentmwillis

trentmwillis Mar 8, 2016

Member

@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.

Member

trentmwillis commented Mar 8, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Mar 8, 2016

Member

@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.

Member

dgeb commented Mar 8, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@trentmwillis

trentmwillis Mar 8, 2016

Member

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.

Member

trentmwillis commented Mar 8, 2016

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

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Mar 9, 2016

Member

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

Member

mixonic commented Mar 9, 2016

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

@MiguelMadero

This comment has been minimized.

Show comment
Hide comment
@MiguelMadero

MiguelMadero Mar 16, 2016

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}}

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}}
Show outdated Hide outdated text/0000-route-serializers.md
}
});
});
```

This comment has been minimized.

@dgeb

dgeb Mar 16, 2016

Member

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 });
});
@dgeb

dgeb Mar 16, 2016

Member

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 });
});

This comment has been minimized.

@trentmwillis

trentmwillis Mar 16, 2016

Member

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

@trentmwillis

trentmwillis Mar 16, 2016

Member

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

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Mar 16, 2016

Member

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}}.

Member

dgeb commented Mar 16, 2016

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

This comment has been minimized.

Show comment
Hide comment
@MiguelMadero

MiguelMadero Mar 16, 2016

Agreed!

On Wed, Mar 16, 2016 at 1:10 PM Dan Gebhardt notifications@github.com
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)

Agreed!

On Wed, Mar 16, 2016 at 1:10 PM Dan Gebhardt notifications@github.com
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

This comment has been minimized.

Show comment
Hide comment
@trentmwillis

trentmwillis Mar 17, 2016

Member

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

Member

trentmwillis commented Mar 17, 2016

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

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Mar 17, 2016

Member

@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 ...

Member

dgeb commented Mar 17, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Mar 17, 2016

Member

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...

Member

rwjblue commented Mar 17, 2016

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

This comment has been minimized.

Show comment
Hide comment
@trentmwillis

trentmwillis Mar 17, 2016

Member

@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.

Member

trentmwillis commented Mar 17, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Mar 18, 2016

Member

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 🎉

Member

mixonic commented Mar 18, 2016

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 🎉

mixonic added a commit that referenced this pull request Mar 18, 2016

@mixonic mixonic merged commit 80ff996 into emberjs:master Mar 18, 2016

@grapho

This comment has been minimized.

Show comment
Hide comment
@grapho

grapho Mar 21, 2016

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?

grapho commented Mar 21, 2016

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

This comment has been minimized.

Show comment
Hide comment
@knownasilya

knownasilya Mar 21, 2016

Contributor

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.

Contributor

knownasilya commented Mar 21, 2016

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment