-
-
Notifications
You must be signed in to change notification settings - Fork 408
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
Add queryParams to the router service #380
Add queryParams to the router service #380
Conversation
I think the RFC is not entirely clear on that it also proposes to make QP's writeable through the service (it does, doesn't it?). If so this would mean that route specific query parameters (e.g. |
it hints at it a bit, but maybe that should be out of scope for now. Based on the behavior you describe, setting a query param would instantly cause a transition -- obviously not ideal. Though, I wonder if in the Transition logging, we could say that a transition was initiated by a query param? I also suggested that this replace the current implementation of query params eventually -- in order to achieve that auto-routing behavior, the logic for that would have to live in some service, rather than in the controller/route? The QP wouldn't be able to live on a route, cause routes don't exist eternally -- though, do controllers? is that why QPs are there currently? maybe the controller just sets up QP usage from the router service? overall, having query params be global, may still simplify the overall understanding of how to work with them. Update (2019-06-09)writable query params have been added |
afaik controllers are Singleton and keep their values (and thus qp’s). |
@NullVoxPopuli Thanks for writing this up, this has been on my mind recently too! I wonder, how do you envisage some of the use cases if QP's are read-only? For example, if a QP is used to manage opening modals, presumably closing a modal should remove that QP, but if it's read-only, the modal component (or modal manager service or whatever) wouldn't be able to directly do that? |
@lougreenwood maybe that depends on usage / defers the readonlyness to app-space? like, export default component SomeComponent exetends Component {
@service router;
@readOnly('router.queryParams.isModalOpen') isModalOpen;
} Though, this could lead to app-devs trolling themselves by declaring it as non-read-only elsewhere in the app. |
It should be pointed out that one can already access the controller for the current route as long as one has access to the router service and the owner. It also suggests a simpler, alternative implementation of the design, to simply alias the properties on the current controller. This would not have a drawback of breaking any existing applications. |
@Gaurav0 aliasing to that on the router service would be a dirt simple implementation. What would the behavior be if a query param isn't found? (or it was on a different controller) just return |
@Gaurav0 Would this assume that the query param were already explicitly defined on the controller, or could this method capture any QP entered as part of the URL? To me, the real power comes when QPs are something like some other state storage mechanism that a component can directly reference/manipulate. For example, I might have a modal component which is used on many routes, the modal may use some QP to toggle it's visibility. Having to define every QP on every controller where the component may or may not be used becomes very cumbersome and IMO fragile. |
Maybe it's out of the scope of this discussion... but thinking more about what QPs provide, could QPs be abstractly described as just some kind of serialised state storage mechanism? Why is this a routing concern at all (seems the route/URL is just the persistence method), it strikes me as an application concern which should possibly exist as some service (similar to By associating QPs with routes & controllers, are we "missing the forest for the trees"? |
I super agree that QPs should be a state storage, rather than a route concern. |
This is a very good question. I do suggest it should be
You don't have to put the query param on every controller, you can just put it on the application controller today and reference that by injecting it where ever you need it. The current system is more flexible than it looks by just reading the guides. |
The current mechanism has no means to ask for the "remainder" of parameters - whatever other parameters exist that haven't been identified explicitly as known properties. Will such a mechanism exist here? |
Yes, because the whole of the query params would be accessible from a singular computed property on the router service. :) That computed property could then allow access to other computed properties representing individual parameters |
It would be great to separate query params from controllers (thanks @NullVoxPopuli for starting this discussion 👏 ). However, I think this is more complicated than this RFC makes it sound like. Instead of an RFC proposing a specific change, I think it would be better to start discussing the set of different options that are available, and what requirements [functionality] need to be supported in a future query params solution. That discussion could start as an issue in this repo (here is an example: #379). It would also be good to loop in the resident query params expert @machty and hear his thoughts! Probably also someone else on the core team, since it's good to sync with other planned functionality. Different options
Various questions
|
@sandstrom I've been generally thinking about this, and whilst my idea isn't fully developed, I'm beginning to wonder if QPs, from an application perspective, could be handled by some state service like I haven't explored the idea in enough depth, but I wonder if that would then allow controllers/routes/components etc to bind to values in the state manager and the handling of the QP specific logic becomes a hidden abstraction from the actual use of QP values? Something like: |
I have personally found query params to be both incredibly important to the kind of apps we work on as well as perhaps a weaker part of the framework. This perception probably comes down to the fact that the apps I work on require an unusual number of query params, sometimes even "dynamically defined" (something currently not possible without serializing custom hashes into the URL). But it's also more concretely that they aren't fully integrated into things like computed properties (you cannot reliably use a CP as a query param, even if you define a custom setter). I believe "QPs-as-a-service" would help improve the usability of QPs by making them easier to inject into components that update based on their state. This improves component reusability by de-coupling them from their controller context and making them easier to configure across different application use cases. On numerous occasions I have needed to pass many query parameters into components. This has led to threading dozens of properties through components when a service might have worked better. Their coupling with routes and controllers has made them hard to inject into re-usable components across applications, as I find controllers and routes to be so domain-specific to a given problem. Other alternatives include Ember Parachute, which provides a QP state object on the controller (rather than as a service). This has been immensely useful and acts as strong evidence in support of this RFC.
On top of this example in the RFC, I also 100% agree with @lougreenwood's points. I have on many occasions needed to bind query param state to individual models, which has led to some wild services full of "forbidden" observers. At one point, I prototyped a custom Location object for handling dynamic query params - but this was hard to do because I could not hook into the internals of how Ember serializes URLs and updates state (probably for good reason).
I thought it is possible currently? We should also loop in @poteto and @offirgolan who have worked on this problem quite a lot. |
It would be good if the source of the QP (i.e. the controllerName) was somehow exposed also. (I totally agree that queryParams are an application level concern and should be treated as such, but that discussion sounds like it's either out of scope for this RFC or would invalidate this RFC). |
Personally, I see no reason why scope couldn't be increased. Bigger changes would just require more planning and include deprecations and interoperability and such. Question though, why would you want the controller name? |
It would be a totally different rfc to make QPs global and not tied to controllers. Of course it's fine to use this particular document to add all of that in, but I think you'd have to take several steps back to have that conversation.
Mainly because the location of the definition is an integral part of how QPs work today. If nothing was changed there, I wouldn't want to lose that context when accessing a QP in some component. It would be weird to "forget" the part of the route hierarchy that a particular QP belongs to, just because you're accessing it in an arbitrary place. |
I must clarify, I mostly just want conversation to grow more freely, in case something important otherwise would be stifled for being out of scope. I do understand that the more that goes in to an RFC, the harder it'll be to get people to agree to it. Maybe https://discuss.emberjs.com would be better for ideas and other things not directly related to what is proposed here. |
@NullVoxPopuli, what do you make of this use case? In some situations, I need to intercept a transition and make sure that its query params are in a valid state. I'd like to abort the transition, extract the query params from it, update/remove/add params as needed, then kick off a new transition. There are two problems with this:
Do you think this RFC could potentially address the need to find out the query params associated with the active transition? |
@magistrula yeah, I'd think this RFC could address that use case. async beforeModel(transition) {
const query = transition.queryParams;
const { name, publicKey } = query;
await this.acceptInvite(name, publicKey);
} with RFC: @service router;
@readOnly('router.queryParams') query;
async beforeModel(transition) {
const { name, publicKey } = this.query;
await this.acceptInvite(name, publicKey);
} Though, my scenario uses query params as read only. @magistrula can you provide some sample code for your scenario? |
Now different models can have different qp for same route. How we can implement it in new design? |
@lifeart, can you provide an example, I don't quite understand what you mean |
@NullVoxPopuli https://ember-twiddle.com/b9e78f44f0a63a1a5b0f75ca28b9ebef?openFiles=controllers.item.js%2C&route=%2F3%3Fstring%3Ddirr%2520serach try to change input value and click on each route and check URL Query params binded to model. step 1
step 2
step 3
step 4
results
|
That's a good question. Since each model is its own route, the route path is changing, and since we've discussed / decided that the query params need to have the route path as one of their keys, I must assume this is possible. I think I personally need to know more about how routing is implemented and how route state is managed, the computed property that this RFC is about would need to hook into that state. |
@NullVoxPopuli, my use case looks something like this: beforeModel(transition) {
const transitionQueryParams = transition.queryParams;
const { foo, bar } = transitionQueryParams;
if (!foo && !bar) {
transition.abort();
return this.transitionTo(transition.targetName, {
queryParams: Object.assign({}, transitionQueryParams, {
foo: 'some-value'
})
}
}
} And the issue I'm running into is that |
I'd also like to see an exploration of why the core has to do this at all, what would a minimal API look like to allow this entire RFC to be implementable in user space? From my perspective, there are a few fundamental things that might unblock that:
I'm willing to accept that this might be naive, but it seems like an interesting (smaller, more customizable, and easier to implement) alternative... |
- All query params, even if not specified are allowed. given the above example, I could use the qp "strongestAvenger" anywhere | ||
- Any sort of transition will clear the query params, unless it is defined as a sticky queryParam. So, if I'm on the posts/new route with the query params "foo" and "baz", and transition to posts/show, those query params are still available. If I navigate to the faq page, foo and baz are cleared from the URL. | ||
- The globally defined query params will stick around until cleared manually. If I visit faq?bar=2, and then transition to posts. the bar=2 query param will still be present. | ||
- The `this.queryParams` function will be available at every nesting of the route tree, but `queryParams` are also configurable in the route options hash. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little concerning. I'd prefer there to be only one way to do this. Maybe it should always be as an option on this.route()
, with an option for whether or not it cascades down to child routes?
this.route('search',
{
queryParams: {
names: ['foo', 'bar', baz'],
cascades: false
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also prefer to avoid adding this.queryParams
to the context of each callback on the Router.map
. Passing configured query params as metadata seems better (and allows better interop with other things that also want to be router map metadata).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another open question from this.queryParams()
is the order of declaration and if that matters. Since the order of declaration of this.route()
does matter, I'd want a spec for what these variations do:
this.queryParams('search');
this.route('foo');
vs:
this.route('foo');
this.queryParams('search');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer there to be only one way to do this.
without this.queryParams
, there'd be no way to set global query params, unless we started adding this.route('application')
by default -- which may be fine
order of declaration and if that matters.
there is no difference in order with this.queryParams
as it's just a function-scoped invocation, and applies to the route-scope of this
(application at the top level)
```ts | ||
@service router; | ||
|
||
this.router.setQueryParam('term', 'RTX'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative here is to have this.router.queryParams.set('term', 'RTX')
. I get that there's value in queryParams
being a POJO, and that introducing a custom type here may introduce hurdles for people implementing their own serialize
/deserialize
hooks, but defining a more strict interface may also be a good thing in the long run (similar to URLSearchParams
, for example)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to stay away from set/get as they're not really present in idiomatic Octane.
but defining a more strict interface may also be a good thing in the long run
I think being simple first is more important here, as the current query params are too restrictive and there are too many unknowns with (de)serialization
I'm gonna split this in to two separate RFCs (coming in the following days)
|
- Make the design a little clearer - specify that the query params must have a dependent key on the route name - add code to "How we teach this" for before and after the RFC
Co-Authored-By: Jan Bobisud <[email protected]>
Co-Authored-By: Robert Jackson <[email protected]>
5971add
to
d42cd00
Compare
Closing this RFC in favor of: |
Rendered