-
-
Notifications
You must be signed in to change notification settings - Fork 405
Arbitrary Query Params #715
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
Closed
NullVoxPopuli
wants to merge
5
commits into
emberjs:master
from
NullVoxPopuli:implicit-application-query-params
Closed
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
363ef30
Add RFC: Implicit Application Query Params
NullVoxPopuli cf51754
Add RFC Link
NullVoxPopuli a312892
Rename RFC file
NullVoxPopuli e56d751
assume option-feature only, add some details
NullVoxPopuli 70f7c57
Add goals, add example about sticky query params
NullVoxPopuli File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,196 @@ | ||
| --- | ||
| Stage: Accepted | ||
| Start Date: 2020-01-30 | ||
| Release Date: Unreleased | ||
| Release Versions: | ||
| ember-source: vX.Y.Z | ||
| ember-data: vX.Y.Z | ||
| Relevant Team(s): Ember.js | ||
| RFC PR: https://github.com/emberjs/rfcs/pull/715 | ||
| --- | ||
|
|
||
| <!--- | ||
| Directions for above: | ||
|
|
||
| Stage: Leave as is | ||
| Start Date: Fill in with today's date, YYYY-MM-DD | ||
| Release Date: Leave as is | ||
| Release Versions: Leave as is | ||
| Relevant Team(s): Fill this in with the [team(s)](README.md#relevant-teams) to which this RFC applies | ||
| RFC PR: Fill this in with the URL for the Proposal RFC PR | ||
| --> | ||
|
|
||
| # Arbitrary Query Params | ||
|
|
||
| ## Summary | ||
|
|
||
| This RFC aims to add opt-in behavior that would allow developers to use *any* | ||
| Query Param passed to `transitionTo`. This will hopefully improve the ergonomics | ||
| of using query params in most default situations. | ||
|
|
||
| ## Motivation | ||
|
|
||
| Query Params require configuration to use today, which is counter to our | ||
| "convention over configuration" philosophy. This RFC proposes an opt-in change to | ||
| allow for query params to _always_ be used in transitions and links, and no longer | ||
| require the allow-list that controllers currently implement today. | ||
|
|
||
| Goals: | ||
| - the set of query param values accessible on the currentRoute are a direct result | ||
| of the URL | ||
| - query param values are never out of sync. | ||
| - with the optional feature flag enabled, controllers do not set values as a | ||
| result of URL transitions | ||
|
|
||
| ## Detailed design | ||
|
|
||
| Arbitrary Query Params will need to be guarded by an optional feature, because: | ||
| - controllers are allow-list only, and the ability to allow all query params | ||
| by default conflicts with the feature set of a controller's allow-list. | ||
| - query param values will not be stored on any controller, whereas the | ||
| controller is the only place tracked-access to query params exists. | ||
|
|
||
|
|
||
| ### Example | ||
|
|
||
| Query Params as they work today are allow-list only, and block all other Query Params. | ||
|
|
||
| Example: | ||
| ```js | ||
| export default class ArticlesController extends Controller { | ||
| queryParams = ['category']; | ||
| } | ||
| ``` | ||
|
|
||
| With `queryParams` specified, only `category` will be allowed on the route for | ||
| this controller. | ||
|
|
||
| This RFC only proposes a change for this scenario: | ||
| ```js | ||
| // Empty or default controller | ||
| export default class ArticlesController extends Controller { | ||
| } | ||
| ``` | ||
|
|
||
| where `<RouterService>#transitionTo({ queryParams: { category: 'anything' } })` | ||
| and LinkTo will _not_ strip away the `category` Query Param. | ||
|
|
||
| ### Migration Path | ||
|
|
||
| 1. Update optional-features.json | ||
|
|
||
| ``` | ||
| // config/optional-features.json | ||
| { | ||
| "arbitrary-query-params": true | ||
| } | ||
| ``` | ||
|
|
||
| 2. Delete Query Param allow lists | ||
|
|
||
| ```diff | ||
| export default class ArticlesController extends Controller { | ||
| - queryParams = ['category']; | ||
| } | ||
|
|
||
| ``` | ||
|
|
||
| ### Examples | ||
|
|
||
| Most "fancy features" of query params would be implemented in user-space or in | ||
| supplemental addons, such as ember-parachute. | ||
|
|
||
| #### Sticky Query Params | ||
|
|
||
| [Demo Ember Twiddle](https://ember-twiddle.com/7e472191b3f5021433b8552158a4379e?openFiles=routes.articles%5C.js%2C&route=%2Farticles) | ||
|
|
||
| > Note that this example has been implemented in Ember 3.18, and would be | ||
| > significantly simpler given the optional feature proposed in this RFC. | ||
|
|
||
| Sticky query params are supported by controllers by default, but with the optional | ||
| feature flag enabled, there is no sticky behavior. Query params are strictly a reflection | ||
| of what is in the URL. To re-implement sticky query params, an addon might have an | ||
| implementation similar to this (and/or the twiddle). | ||
|
|
||
| ```ts | ||
| // app/services/query-params.ts | ||
| const CACHE = new Map<Record<string, string>>(); | ||
|
|
||
| function getForUrl(url: string) { | ||
| let existing = CACHE.get(url); | ||
|
|
||
| if (!existing) { | ||
| CACHE.set(url, {}); | ||
|
|
||
| return existing; | ||
| } | ||
|
|
||
| return existing; | ||
| } | ||
|
|
||
| class QueryParamsService extends Service { | ||
| @service router; | ||
|
|
||
| setQP(qpName, value) { | ||
| let cacheForUrl = getForUrl(this.router.currentURL); | ||
|
|
||
| cacheForUrl[qpName] = value; | ||
| } | ||
|
|
||
| getQP(qpName) { | ||
| return getForUrl(this.router.currentURL)[qpName]; | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ```js | ||
| // some route | ||
| export default MyRoute extends Route { | ||
| @service router; | ||
| @service queryParams; | ||
|
|
||
| async beforeModel({ to: { queryParams }}) { | ||
| let category = this.queryParams.getQP('category'); | ||
|
|
||
| // if transitioning to a route with explicit query params, | ||
| // update the query param cache | ||
| if (stickyQPs.category && category !== queryParams.category) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where did this |
||
| this.queryParams.setQP('category', queryParams.category); | ||
| } | ||
|
|
||
| // because the URL must reflect the state query params, we must transition | ||
| if (!queryParams.category && category) { | ||
| this.router.transitionTo({ queryParams: { category }}); | ||
| } | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
|
|
||
| ## How we teach this | ||
|
|
||
| The current guides would need updating teach that query params could be used without | ||
| modifying controllers. While the existing Query Params behavior would still work, | ||
| and provide much more utility than always arbitrary query params, it would be more | ||
| of an "Advanced Topic". | ||
|
|
||
|
|
||
| ## Drawbacks | ||
|
|
||
| - Updating the guides would probably be easier if done in conjunction with | ||
| [RFC#712: Query Params as derived state](https://github.com/emberjs/rfcs/pull/712) | ||
|
|
||
| - Adding an optional feature increases test matrices and adds overall complexity | ||
| to the routing system, but changing the default behavior of `queryParams = []` | ||
| (or unspecified) could break people's apps. | ||
|
|
||
| ## Alternatives | ||
|
|
||
| > What other designs have been considered? What is the impact of not doing this? | ||
|
|
||
| > This section could also include prior art, that is, how other frameworks in the same domain have solved this problem. | ||
|
|
||
| ## Unresolved questions | ||
|
|
||
| > Optional, but suggested for first drafts. What parts of the design are still | ||
| TBD? | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
There is some maybe undefined behavior around not having this specified atm.
For example,
this.router.currentRoute.queryParamsis still updated with transitions, but all QPs are stripped from the URL.This RFC, to put it simply, makes all things equal:
That's it.
The first statement is not true today, and is a huge source of weird
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.
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)