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
Web API: URLSearchParams #1049
Web API: URLSearchParams #1049
Conversation
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.
Cool thanks - looks good - just one comment
BUILD.gn
Outdated
@@ -107,6 +107,7 @@ ts_sources = [ | |||
"js/trace.ts", | |||
"js/truncate.ts", | |||
"js/types.ts", | |||
"js/urlsearchparams.ts", |
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.
url_search_params.ts
js/urlsearchparams_test.ts
Outdated
// Copyright 2018 the Deno authors. All rights reserved. MIT license. | ||
import { test, assert, assertEqual } from "./test_util.ts"; | ||
|
||
test(function initString() { |
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.
Can you prefix call of the test function names with “url” or “urlSearchParams”
This is so we can easily filter
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.
Yes, fixed in next commit 👍
js/urlsearchparams.ts
Outdated
|
||
private params: [string, string][] = []; | ||
|
||
public constructor(init: string | Iterable<[string, string]> | Record<string, string> = '') { |
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.
init
may be undefined
var u = new URLSearchParams()
undefined
u instanceof URLSearchParams
true
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.
The =
at the end states it's optional. It defaults to ''
to comply with WHATWG.
js/urlsearchparams.ts
Outdated
* searchParams.append('name', 'first'); | ||
* searchParams.append('name', 'second'); | ||
*/ | ||
public append(name: string, value: string): void { |
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.
public
is unnecessary
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.
Fixed this and all other occurrences in the next commit, thanks you 👍
js/urlsearchparams.ts
Outdated
* | ||
* searchParams.delete('name'); | ||
*/ | ||
public delete(name: string): void { |
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.
Ditto
js/urlsearchparams.ts
Outdated
* | ||
* searchParams.getAll('name'); | ||
*/ | ||
public getAll(name: string): string[] { |
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.
Ditto
js/urlsearchparams.ts
Outdated
* | ||
* searchParams.get('name'); | ||
*/ | ||
public get(name: string): string | null { |
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.
Ditto
js/urlsearchparams.ts
Outdated
* | ||
* for (const key of searchParams.keys()) console.log(key); | ||
*/ | ||
public *keys(): Iterable<string> { |
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.
Ditto
js/urlsearchparams.ts
Outdated
* | ||
* for (const value of searchParams.values()) console.log(value); | ||
*/ | ||
public *values(): Iterable<string> { |
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.
Ditto
js/urlsearchparams.ts
Outdated
* | ||
* for (const [key, value] of searchParams.entries()) console.log(key, value); | ||
*/ | ||
public *entries(): Iterable<[string, string]> { |
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.
Ditto
js/urlsearchparams.ts
Outdated
* | ||
* for (const [key, value] of searchParams) console.log(key, value); | ||
*/ | ||
public *[Symbol.iterator](): Iterable<[string, string]> { |
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.
Ditto
js/urlsearchparams.ts
Outdated
* | ||
* searchParams.toString(); | ||
*/ | ||
public toString(): string { |
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.
DItto
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.
LGTM
export class URLSearchParams { | ||
private params: Array<[string, string]> = []; | ||
|
||
constructor(init: string | string[][] | Record<string, string> = "") { |
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.
If this spec lists this as optional shouldn't this be init?:
export class URLSearchParams { | ||
private params: Array<[string, string]> = []; | ||
|
||
constructor(init: string | string[][] | Record<string, string> = "") { |
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.
If this spec lists this as optional shouldn't this be init?:
export class URLSearchParams { | ||
private params: Array<[string, string]> = []; | ||
|
||
constructor(init: string | string[][] | Record<string, string> = "") { |
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.
If this spec lists this as optional shouldn't this be init?:
export class URLSearchParams { | ||
private params: Array<[string, string]> = []; | ||
|
||
constructor(init: string | string[][] | Record<string, string> = "") { |
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.
If this spec lists this as optional shouldn't this be init?:
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.
just a question from this merge
- Add URLSearchParams (denoland#1049) - Implement clone for FetchResponse (denoland#1054) - Use content-type headers when importing from URLs. (denoland#1020) - Use checkJs option, JavaScript will be type checked and users can supply JSDoc type annotations that will be enforced by Deno (denoland#1068) - Add separate http/https cache dirs to DENO_DIR (denoland#971) - Support https in fetch. (denoland#1100) - Add chmod/chmodSync on unix (denoland#1088) - Remove broken features: --deps and trace() (denoland#1103) - Ergonomics: Prompt TTY for permission escalation (denoland#1081)
- Add URLSearchParams (#1049) - Implement clone for FetchResponse (#1054) - Use content-type headers when importing from URLs. (#1020) - Use checkJs option, JavaScript will be type checked and users can supply JSDoc type annotations that will be enforced by Deno (#1068) - Add separate http/https cache dirs to DENO_DIR (#971) - Support https in fetch. (#1100) - Add chmod/chmodSync on unix (#1088) - Remove broken features: --deps and trace() (#1103) - Ergonomics: Prompt TTY for permission escalation (#1081)
This PR implements URLSearchParams with tests. The code has been successfully formatted, linted, and tested.