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

Query Params as derived data #712

Closed

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Jan 26, 2021

rendered

This RFC is part of a series of RFCs to lead to the deprecation of controllers entirely. For more about how I imagine that happening, let's talk on Discord / keep the RFCs themselves focused on the topic they're about.

Related RFCs:

@NullVoxPopuli NullVoxPopuli changed the title Query Params as derived data WIP: Query Params as derived data Jan 26, 2021
text/0712-query-params-as-derived-data.md Outdated Show resolved Hide resolved
text/0712-query-params-as-derived-data.md Outdated Show resolved Hide resolved
export default class ArticlesController extends Controller {
@service router;

queryParams = ['category'];
Copy link

Choose a reason for hiding this comment

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

I like this direction. Do you consider how query params are declared in scope for this RFC?

Copy link
Contributor Author

@NullVoxPopuli NullVoxPopuli Jan 29, 2021

Choose a reason for hiding this comment

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

Maybe discussing them is -- as it's def part of the learning story! :)

I also don't think query params should have default values (especially since that state is entangled with the controller).
For the URL to be the source of truth, to access a QP from within a component would require something like this:

get category() {
  return this.router.currentRoute.queryParams.category ?? 'default value';
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

related: #715

@locks locks added the T-ember-data RFCs that impact the ember-data library label Feb 22, 2021
@NullVoxPopuli NullVoxPopuli changed the title WIP: Query Params as derived data Query Params as derived data Mar 14, 2021
@sly7-7
Copy link

sly7-7 commented Mar 15, 2021

I've quickly read (maybe too quick), but I can't see what happen to the { scope: 'controller' } thing.

@NullVoxPopuli
Copy link
Contributor Author

I've quickly read (maybe too quick), but I can't see what happen to the { scope: 'controller' } thing.

the setting doesn't affect the derived data patterns -- here is an example: https://ember-twiddle.com/567b7acf47448cee1f63fcb36e82cd66?openFiles=controllers.articles%5C.js%2C

…-- the sticky query params example is more applicable to emberjs#715
@wagenet
Copy link
Member

wagenet commented Jul 23, 2022

@NullVoxPopuli is this just waiting on core review?

@NullVoxPopuli
Copy link
Contributor Author

yup

@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
@wagenet wagenet closed this Dec 16, 2022
@NullVoxPopuli
Copy link
Contributor Author

Fwiw, this RFC still serves as a source of (imo) better documentation for managing query params than we have today in the guides.

(this was a documentation only RFC)

@NullVoxPopuli
Copy link
Contributor Author

let { href } = this.args;
let qps = stickyQPsToQueryString(this.queryParams.forUrl(href));

return `${href}?${qps}`;
Copy link

Choose a reason for hiding this comment

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

This is mixing the URL APIs and string concatenation.

I'd love to see Ember support the Web standard URL APIs for this.

Something like this would gain the benifits of proper URI encoding.

let url = new URL(this.args.href, 'thismessage:/');
url.search = stickyQPsToQueryString();
return url.href;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, the example is out of date -- using URL is very good, and I wrote all this before I started using URL, URLSearchParams, etc.

This is all userland code tho, so folks should feel free to use URL 🎉

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.

10 participants