Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Arbitrary Query Params #715

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Jan 30, 2021

rendered

Side-effecting goals by not defaulting to sticky params:

  • LinkTo / navigations can remove many query params all at once (currently this is only possible by specifying every query param in your link-to and setting to default value)
  • ...

Example of what happens today when the queryParams array is omitted:
secret-query-params
(Twiddle is Ember 3.18)

@sandstrom
Copy link
Contributor

How about sticky query params? Wouldn't exist in this mode, I assume, but what would a user-space implementation look like?

Same question for "default query params".

When doing refresh on a route, would the query params stay around? (and would the route model hook receive them)

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Feb 18, 2021

How about sticky query params? Wouldn't exist in this mode, I assume, but what would a user-space implementation look like?

Correct!

I'm thinking something like this:

// this could also live on some service
const stickyQPs = {};

export default MyRoute extends Route {
  @service router;
  
  async beforeModel({ to: { queryParams }}) {
    if (!stickyQPs.category) {
      stickyQPs.category = queryParams.category;
    }

    if (!queryParams.category) {
      this.router.transitionTo({ queryParams: { category: 'default value' }});
    }
  }

Same question for "default query params".

export default MyRoute extends Route {
  @service router;
  
  async beforeModel({ to: { queryParams }}) {
    if (!queryParams.category) {
      this.router.transitionTo({ queryParams: { category: 'default value' }});
    }
  }

When doing refresh on a route, would the query params stay around? (and would the route model hook receive them)

yup, the transition.to object has the intended query params on the URL -- as long as a transition away from those query params hasn't happened, they'll be available in the route model hooks


I should add these examples to here: #712

@NullVoxPopuli NullVoxPopuli changed the title Implicit Application Query Params Arbitrary Application Query Params Feb 18, 2021
@NullVoxPopuli NullVoxPopuli changed the title Arbitrary Application Query Params Arbitrary Query Params Feb 18, 2021
@sandstrom
Copy link
Contributor

sandstrom commented Feb 19, 2021

Some followup questions:

With sticky, how would that work with links? A {{link-to wouldn't know that the remembered/sticky query param for route /users is ?sort=last-name. Basically, you couldn't render <a href="/users?sort=last-name">Users</a> since you won't know the sticky query param value before hand. I think that's a fairly big drawback.

With default query params, you'd get the link <a href='/stores'>View stores</a> but navigating into it would yield /stores?filtering=active. With current default query params behavior you'd get /stores but the query param property filtering would return 'active' as its default value (which would not be rendered to the url). I think this is less of a problem though, since it could be handled via e.g. a getter that converted null/undefined into the string 'active', such that the difference [between your proposal and existing QPs] would be invisible to the rest of the controller/route.

Also, triggering this.router.transitionTo in beforeModel hooks works, but behavior that rely on transitions to indicate some type of state change (analytics software, etc) would get confused. Seems like we're hijacking a method that's really meant for one thing (state changes) and using it fore something else (setting default query params or handling sticky query params). I'm not convinced that's a good idea.

I don't really know what I think of the overall proposal (I'm neither for nor against yet), but wanted to highlight some possible problems.

I agree with you that decoupling query params from controllers would be great. It's awesome that we're thinking about different ways of getting rid of them.

@NullVoxPopuli
Copy link
Contributor Author

With sticky, how would that work with links?

I think you'd want to make your own Link component and store the sticky data in a service and have your custom link component reference that service.

With current default query params behavior you'd get /stores but the query param property filtering would return 'active' as its default value (which would not be rendered to the url).
I think this is less of a problem though, since it could be handled via e.g. a getter that converted null/undefined into the string 'active', such that the difference [between your proposal and existing QPs] would be invisible to the rest of the controller/route.

yeah, I don't think we should ever have QP state that isn't represented in the URL, so what you propose is totally valid, and I think the behavior absolutely should be left up to folks to decide for themselves in their apps :D

Seems like we're hijacking a method that's really meant for one thing (state changes) and using it fore something else (setting default query params or handling sticky query params)

the URL, including query params, are the state though -- but that's just a thing with my proposed whipped up implementation. If someone didn't want to transition, that's fine, too.

However, I agree with you that decoupling query params from controllers would be great. It's awesome that we're thinking about different ways of getting rid of them.

Yeah, this is really the last thing we need to stop using controllers, allowing for better composition patterns (I know this is subjective, but we can talk about that on Discord :p ).
I don't think we need to provide a built-in solution for all the behavior today, and I think that scaling back the feature set, behind a flag / optional feature, will allow more flexibility for future iterations -- but in app/addon space.


```diff
export default class ArticlesController extends Controller {
- queryParams = ['category'];
Copy link
Contributor Author

@NullVoxPopuli NullVoxPopuli Mar 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is some maybe undefined behavior around not having this specified atm.

For example, this.router.currentRoute.queryParams is still updated with transitions, but all QPs are stripped from the URL.

This RFC, to put it simply, makes all things equal:

  • transitionTo matches the URL and router service state
  • does not set properties on controllers

That's it.

The first statement is not true today, and is a huge source of weird

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a demonstration of what happens today when the query params array is omitted: https://ember-twiddle.com/7e472191b3f5021433b8552158a4379e?openFiles=routes.articles%5C.js%2C&route=%2Farticles

(this demo mostly demonstrates how to manage your own sticky params with Ember 3.18, but there is weird router behavior demonstrated, too)

NullVoxPopuli added a commit to NullVoxPopuli/rfcs that referenced this pull request Mar 15, 2021
…-- the sticky query params example is more applicable to emberjs#715
@gnclmorais
Copy link

Coming from a more recent Ruby on Rails background, this change makes sense to me. You have access to all params on a Rails controller, which is usually quite useful.

However, I would would also like to ask/suggest if we could find a corresponding to Rail’s strong parameters (either by building something into the framework or suggest a good example of how to deny/allowlist parameters). Maybe together with #712, we could provide a documentation example of how to derive safe parameters (i.e. filtering out random parameters that we don’t want to pass on), something like this:

import Component from '@glimmer/component';
import { action } from '@ember/object';
import { inject as service } from '@ember/service';

export default class MyDeeplyNestedComponent extends Component {
  @service router;

  get params() {
    const { page, sort, category } = this.router.currentRoute.queryParams;

    return { page, sort, category };
  }
}

@sandstrom
Copy link
Contributor

@gnclmorais the threat model for a single page app is somewhat different from Rails, so I don't know if something like strong params would necessarily make sense here. Common vectors are XSS, CSRF and a few others.

Sure, one can certainly make up a scenario where non-sanitized urls could allow an attacker to e.g. trick another user into doing something harmful, but params are overall much less sensitive on the front-end.

@gnclmorais
Copy link

You’re right @sandstrom, it’s not the same on the frontend side — I was providing a parallel with Rails mostly as a documentation example, a way we can put this PR and #712 together to provide more clarity.

Removing the queryParams = ['category'] bit will, in a way, remove documentation from the framework — they become less explicit in showing “these are the params we accept”, so the snippet I provided looked like a clean, self-documenting way of saying “from all the params we can get, these are the ones we want to handle”.

@sandstrom
Copy link
Contributor

@gnclmorais I agree regarding documentation, self-documenting code would definitely be useful + agree that whitelisting params is a good idea (but not primarily for security).


// if transitioning to a route with explicit query params,
// update the query param cache
if (stickyQPs.category && category !== queryParams.category) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did this stickyQPs comes from? Is this a typo?

@wagenet
Copy link
Member

wagenet commented Jul 23, 2022

@NullVoxPopuli where does this stand?

@NullVoxPopuli
Copy link
Contributor Author

It's something I think we absolutely need as a framework, but the timing of the concept of arbitrary query params may fit better with whatever overhauls the routing system may get with/post polaris. Would be good to talk to / hear from core about what's planned around that.

My primary belief around QPs is that opt-in by default is backwards, and this RFC reverses that.

@chriskrycho chriskrycho added the S-Proposed In the Proposed Stage label Dec 1, 2022
@wagenet
Copy link
Member

wagenet commented Dec 6, 2022

Thanks for spending the time writing this RFC! Given the significant number of concerns around routing, we’re going to be completely revamping the router as part of Polaris. This RFC won’t directly apply to the new router so we’re going to move this PR to FCP to Close. But don’t worry, your work here was not in vain! We will be keeping track of this RFC and others like it to make sure that the concerns it aims to address will be covered in the new router. Your work here does a significant service to us in this regard!

@wagenet wagenet added the FCP to close The core-team that owns this RFC has moved that this RFC be closed. label Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FCP to close The core-team that owns this RFC has moved that this RFC be closed. S-Proposed In the Proposed Stage T-routing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants