Manage query parameter at route only

[jelhan]

Query parameters are managed at two places in Ember today: At the controller and it's corresponding route.

This has two main downsides in my opinion:

1. It increases mental overhead as a developer has to recall which configuration goes to the controller and which goes to the route.
2. Managing query parameters is the only reason left for which controllers must be used. This prevents teams to fully migrate away from controllers if they prefer to do so.

How Ember handles query parameter today has several issues. Solving all of them will very likely require a rework of the entire feature. While I think doing so is needed at one point in time, I think it shouldn't block us from shipping small improvements step by step. Making query parameter not blocking migrating away from controllers is a valuable first step in my opinion.

We could allow teams to migrate away from controllers by moving existing query parameter registration and configuration from controller to the route - without requiring any other change to existing feature.

Beside registration and configuration of a query parameter the controller is often used to read and mutated query parameters:

class MyController extend Controller {
  queryParams = ["sort"];
  sort = "asc";

  // read query parameter value
  get sortedModel() {
    const { sort } = this;
    const sortedModel = [...this.model].sort();

    if (sort = "desc") {
      sortedModel.reverse();
    }

    return sortedModel;
  }
  
  // mutate query paramter value
  @action
  updateSorting(sort) {
    this.sort = sort;
  }
}

Thanks to the router service query parameter can be read and mutated everywhere - not only through the controller. Interacting with query parameters through RouterService does not only allow more flexible architecture. It also makes it explicit that query parameters are changed by a transition - even though that transition may not change current route nor its dynamic segments. This also simplifies the mental model of routing in Ember.

class MyComponent extend Component {
  @service router;

  // read query parameter value
  get sortedModel() {
    const { sort } = this.router.currentRoute.queryParams;
    const sortedModel = [...this.model].sort();

    if (this.sort = "desc") {
      sortedModel.reverse();
    }

    return sortedModel;
  }

  // mutate query paramter value
  @action
  updateSorting(sort) {
    this.router.transitionTo({
      queryParams: { sort }
    });
  }
}

I'm not sure yet how to deal with the default value of a query parameter. Currently a developer can define a default value for a query parameter by providing a default value for a class property of the controller with the same name:

class MyController extend Controller {
  queryParams = ["sort"];
  sort = "asc";
}

This API isn't very intuitive to me. It could be replaced by a defaultValue option on the query parameter configuration object. But to be honest I'm not sure if it is needed at all. In my opinion it should be the responsibility of the code, which uses the query parameter value to handle the case that query parameter is not set.

I'm opening this issue to see what community and core team thinks before investing time into writing a fully fledged RFC. Would love to hear your thoughts.

[sandstrom]

Overall a good idea!

This API isn't very intuitive to me. It could be replaced by a defaultValue option on the query parameter configuration object. But to be honest I'm not sure if it is needed at all. In my opinion it should be the responsibility of the code, which uses the query parameter value to handle the case that query parameter is not set.

Agree, this is weird. In my experience, having the query params value "mirrored" in both the page url (router state) and on a controller property is also a source of some timing issues, where e.g. mid transition the value may have changed in one location, but not yet have "mirrored" over into the other.

You've probably thought about it, but there is also the case of sticky query params.

Not sure if they are worth preserving/supporting from the route though.

May be better if dev manually stores sticky data in localStorage when leaving the route, and reading it when re-entering, doing an extra transition re-setting those sticky QPs via a transitionTo from beforeModel.

[jelhan]

You've probably thought about it, but there is also the case of sticky query params.

Not sure if they are worth preserving/supporting from the route though.

May be better if dev manually stores sticky data in localStorage when leaving the route, and reading it when re-entering, doing an extra transition re-setting those sticky QPs via a transitionTo from beforeModel.

I fully agree that it feels odd that query parameters are sticky by default. It very likely does not match what a developer expects unless being familiar with Ember routing. But I would keep that feature for now.

Removing that feature will make migration for existing applications way more difficult. It is very likely that user experience highly depends on that feature. And I fear we couldn't codemod the migration to an alternative pattern.

Also it doesn't seem to be obvious what that alternative pattern should be. localStorage is shared among all instances of the application running in different tabs of the same browser. It is also picked up when user refreshes the page. That is very different from existing behavior.

sessionStorage would be an alternative. It is not shared among different instances running in separate tabs. But it survives a full page refresh. While that's maybe a better user experience compared to what we have today in many cases, the migration would have an affect on user experience. Teams would need to consider on a case by case basis, if that is desired.

A service might allow us to replicate the existing feature in a transparent way. But as discussed before other migration targets may fit actual use case better.

All in all I think the question of stick query parameters and how to move forward with them, deserves its own RFC due to complexity.

Maybe as a tiny step towards that, we could switch the default for query parameters defined on the route. Meaning that a query parameter would not be sticky by default but a developer must opt-in to that feature. That would also make opt-out easier than today. And provide an easier path-forward for deprecating that feature. An interface for it may look like the following:

export interface RouteQueryParam {
    sticky?: 'route' | 'model' | undefined;
}

• 'model' would map to the current default.
• 'route' would map to the existing { sticky: 'controller' } option.
• undefined would be the same as resetting the query parameter in resetController hook manually.

[sandstrom]

Good points!

Switching the default could make sense, even if that functionality remain in the long term, having it opt-in would make sense.

Not sure I understand the last part about the three different modes, I thought there were only two (sticky and non-sticky).

(but you don't have to explain, that'll probably be more clear when an RFC is written, and I agree with what you wrote, so I don't need any convincing 😀)

Overall I think this sounds like a great idea. Good with a small scope, and would pave way for removing controllers for those that want to.

[jelhan]

Not sure I understand the last part about the three different modes, I thought there were only two (sticky and non-sticky).

Ember currently supports two different modes for sticky:

In some cases, you might not want the sticky query param value to be scoped to the route's model but would rather reuse a query param's value even as a route's model changes. This can be accomplished by setting the scope option to "controller" within the controller's queryParams config hash:

https://guides.emberjs.com/release/routing/query-params/#toc_sticky-query-param-values

I noticed that I messed up in my last comment what is the current default for sticky query parameters and what the { sticky: 'controller' } option does. I corrected it in that comment.

[wagenet]

The router needs a lot of work, my concern here is how we get from this to an actual RFC. Any thoughts?

[NullVoxPopuli]

I think at the moment, it's already possible to avoid controllers entirely -- even for query params -- here is a demo: https://github.com/NullVoxPopuli/ember-demo-route-based-query-params-config

[wagenet]

@NullVoxPopuli should this be closed then? or is there more worth keeping here?

[NullVoxPopuli]

someone could probably update the docs here: https://github.com/ember-learn/guides-source

but, I don't think there is anything left for this RFC issue

[jelhan]

I think at the moment, it's already possible to avoid controllers entirely -- even for query params -- here is a demo: https://github.com/NullVoxPopuli/ember-demo-route-based-query-params-config

I'm not sure if this is public API. It may rely on implementation details only, which could be changed at any time. May need a clarification from framework team.

someone could probably update the docs here: https://github.com/ember-learn/guides-source

We need to clarify first, if this is public API or an implementation detail. Even if it is already public API, we may need an RFC as updating the guides would change how we teach query parameters in a significant way.

but, I don't think there is anything left for this RFC issue

I think input from framework and maybe learning team would be needed on the following two questions to make that decision:

1. Is declaring query parameters in route only public API?
2. Could the guides be updated to teach registering query parameters on route rather than on controller without a RFC?

A RFC is needed if the answer to any of these two questions is no.

[jelhan]

The router needs a lot of work, my concern here is how we get from this to an actual RFC. Any thoughts?

I got feedback from a framework team member early this year that it is unlikely to land any RFC changing existing routing behavior currently. It sounded as framework team wants to have a clear vision for a Polaris router before touching existing stuff. Mainly to avoid unnecessary churn. I paused all activities on this topic due to that feedback.

[wagenet]

@jelhan I was just on a core team meeting where the router stuff was being discussed. We want to be clear that active progress is being made here and very soon we should have some more public information and be able to help rope the community in a bit more!