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

feat(ajax): add params query string parameter configuration #6174

Merged
merged 4 commits into from
Mar 28, 2021

Conversation

benlesh
Copy link
Member

@benlesh benlesh commented Mar 24, 2021

Adds a feature that will accept a wide variety of possible inputs for params to set the query string parameters for ajax.

Note that this feature will require a polyfill for URLSearchParams in IE, but this is not a breaking change for IE, as if they do not use this feature URLSearchParams is never accessed. Adds documentation and relatively thorough tests.

Related to #5384

throw new TypeError('url is required');
}

if (params) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Everything inside of this conditional is the bulk of what was added. Please notice that it's mostly comments, and not actually a lot of code. Also worth noticing that URLSearchParams is only ever accessed inside of this conditional.

Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

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

LGTM. Just a nitpick and what looks like a copy/paste error in a test. Approving this 'cause the latter needs no discussion.

src/internal/ajax/types.ts Outdated Show resolved Hide resolved
spec/observables/dom/ajax-spec.ts Outdated Show resolved Hide resolved
@benlesh benlesh merged commit 980f4d4 into ReactiveX:master Mar 28, 2021
@jayphelps
Copy link
Member

Late to the party here, but note that “params” can also mean (and IMO more often refers to) route params a la Express ‘/products/:productId’ Express’s API calls these ‘params’. Angular and many others do as well.

I suggest naming it query, queryParams, search, or searchParams, to follow naming conventions used elsewhere. My gut says queryParams is probably the right choice since that’s what Angular uses: https://angular.io/api/router/NavigationExtras

@cartant
Copy link
Collaborator

cartant commented Mar 31, 2021

@benlesh +1 from me for queryParams

@benlesh
Copy link
Member Author

benlesh commented Mar 31, 2021

I'm out this morning, but if someone puts a PR in to change it, I'm down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants