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

URLSearchParams API should accept numbers for values #1568

Open
artyil opened this issue Aug 17, 2019 · 18 comments
Open

URLSearchParams API should accept numbers for values #1568

artyil opened this issue Aug 17, 2019 · 18 comments
Labels

Comments

@artyil
Copy link

artyil commented Aug 17, 2019

Actual implementation of URLSearchParams.constructor and URLSearchParams.prototype.set methods in the browsers will accept any value as value, including number or Integer.

TLDR:
new URLSearchParams([['foo', 1]]) and new URLSearchParams().set('bar', 1) should compile without TS errors

It works in current implementation, because numeric value will be coerced to actual USVString value, which is a string.

Unfortunately, current TS definition of type URLSearchParams in lid.dom.d.ts will only allow strings for above mentioned setters. Below are the definitions.

I can understand that the definitions are mirroring the W3C specifications but I do believe it is not the expected experience for developers.

The actual definition enforces developers to make unnecessary operations (helpers / utils / map-reduces) to overcome TS errors 👋

Can we reconsider to allow values to be numeric to mentioned setters? This won't break any current bahavior or actual implementation.

Even as per MDN examples constructor and URLSearchParams.prototype.set it should work as expected:

// Pass in a sequence
var params3 = new URLSearchParams([["foo", 1],["bar", 2]]);

//Add a third parameter.
params.set('baz', 3);

Which works in browser but doesn't work in TS.

TypeScript Version: 3.4.3
output with tsc typescript@next

a.ts:3:34 - error TS2345: Argument of type '{ foo: number; bar: string; }' is not assignable to parameter of type 'string | Record<string, string> | URLSearchParams | string[][]'.
  Type '{ foo: number; bar: string; }' is not assignable to type 'string'.

3 const url2 = new URLSearchParams({ foo: 1, bar: '1' }) // <- FAILS
                                   ~~~~~~~~~~~~~~~~~~~~

a.ts:5:34 - error TS2345: Argument of type '(string | number)[][]' is not assignable to parameter of type 'string | Record<string, string> | URLSearchParams | string[][]'.
  Type '(string | number)[][]' is not assignable to type 'string[][]'.
    Type '(string | number)[]' is not assignable to type 'string[]'.
      Type 'string | number' is not assignable to type 'string'.
        Type 'number' is not assignable to type 'string'.

5 const url3 = new URLSearchParams([['foo', 1], ['bar', '1']]) // <- FAILS
                                   ~~~~~~~~~~~~~~~~~~~~~~~~~~

a.ts:8:17 - error TS2345: Argument of type '1' is not assignable to parameter of type 'string'.

8 url4.set('foo', 1); // <- FAILS
                  ~


Found 3 errors.

Search Terms:
Typescript URLSearchParams invalid constructor
Typescript URLSearchParams won't accept numbers as value
Typescript URLSearchParams won't accept numeric values
standard lib.dom.d.ts URLSearchParams allow numbers as values
Typescript URLSearchParams coercion number to string

Code

const url = new URLSearchParams({foo: 'bar'}) // <- OK 

const url2 = new URLSearchParams({ foo: 1, bar: '1' }) // <- FAILS

const url3 = new URLSearchParams([['foo', 1], ['bar', '1']]) // <- FAILS

const url4 = new URLSearchParams([['bar', '2']]) // <- OK
url4.set('foo', 1); // <- FAILS

Expected behavior:
All above cases should work, because numeric values are being coerced to strings

Actual behavior:
TS won't compile due to URLSearchParams definitions only accept string for value

interface URLSearchParams {
    /**
     * Appends a specified key/value pair as a new search parameter.
     */
    append(name: string, value: string): void;
    /**
     * Deletes the given search parameter, and its associated value, from the list of all search parameters.
     */
    delete(name: string): void;
    /**
     * Returns the first value associated to the given search parameter.
     */
    get(name: string): string | null;
    /**
     * Returns all the values association with a given search parameter.
     */
    getAll(name: string): string[];
    /**
     * Returns a Boolean indicating if such a search parameter exists.
     */
    has(name: string): boolean;
    /**
     * Sets the value associated to a given search parameter to the given value. If there were several values, delete the others.
     */
    set(name: string, value: string): void;
    sort(): void;
    forEach(callbackfn: (value: string, key: string, parent: URLSearchParams) => void, thisArg?: any): void;
}

declare var URLSearchParams: {
    prototype: URLSearchParams;
    new(init?: string[][] | Record<string, string> | string | URLSearchParams): URLSearchParams;
};

Playground Link:
link

Related Issues:
microsoft/TypeScript#15338

@nmain
Copy link

nmain commented Aug 20, 2019

I personally would prefer not to see this change. The automatic coercion of non-strings to strings that exists in many DOM apis is very implicit and I like that typescript is more strict about it, instead forcing me to explicitly Number() or .toString() a numeric that is going to be used as a string.

@CaptainOfPhB
Copy link

I think use .toString() is better, no need to change TypeScript.

@artyil
Copy link
Author

artyil commented Sep 2, 2019

@nmain @CaptainInPHW I truly believe that it doesn't make any sense to justify anything in this thread with a personal preference(s). If you would like to open a discussion please provide solutions to real word use cases I mentioned above

@segevofer
Copy link

I actually agree with @artyil
This example also demonstrate the issue:

const url1 = new URLSearchParams({ foo: false, bar: '1' }) // <- FAILS
const url2 = new URLSearchParams({ foo: 'false', bar: '1' }) // <- OK

The spec of URLSearchParams doesn't specify the value type of that object,

Forcing code like this, is too strict, and weird:

const url1 = new URLSearchParams({ foo: false.toString() })

So it should support string|number|boolean

@BMCooley
Copy link

I largely agree with @nmain that the automatic coercion of non-strings to strings that exists in many DOM apis is very implicit and typescript should NOT cater to those implicit coercions.

I'm not sure that applies here. Here we are creating a new URL object. I think we can agree that a URL is a specific type of string so passing nonString values to the constructor, append and set methods would be like calling a urlify method. You are explicitly asking for a standardized way of converting values to a url type value (string). Very similar to JSON.stringify, yes things are being casted but that's the core functionality of the method.

The underlying implementation just uses template literals to cast to a string. I'd be happy to make the pr to accept template-literable types as values here.

@OliverJAsh
Copy link

OliverJAsh commented Jan 3, 2020

AFAICS, string coercion is not standardised as part of URLSearchParams, so the current types correctly reflect what is (currently) the standard.

constructor(optional (sequence<sequence<USVString>> or record<USVString, USVString> or USVString) init = "");

void set(USVString name, USVString value);

https://url.spec.whatwg.org/#idl-index

I would prefer URLSearchParams adheres to the standard (as it does now). If people want to allow coercion, they can easily compose/wrap URLSearchParams:

const myUrlSearchParams = (init: Record<string, string | number | boolean>) =>
  new URLSearchParams(init as Record<string, string>)

Maybe coercion will be added to the standard in the future, at which point I would expect the types to also be updated.

Not all string coercion is desired (e.g. objects/functions, null, undefined), so the current types will be useful in helping to catch those bugs.

@lucacasonato
Copy link

lucacasonato commented Apr 25, 2021

@RyanCavanaugh I don't think this is a bug, and thus the issue should be closed:

https://url.spec.whatwg.org/#ref-for-dom-urlsearchparams-urlsearchparams specifies that URLSearchParam takes a webidl union of sequence<sequence<USVString>> or record<USVString, USVString> or USVString. In the case specified by the OP the record<USVString, USVString> is used because the webidl union resolution determines that ECMA type of a literal object is "Object", it is not null, and it does not have a @@iterator property (see https://heycam.github.io/webidl/#es-union, mainly 8.2 and 8.4). The reason a number works, is because WebIDL specifies that everything but symbols should be coerced into a string (https://heycam.github.io/webidl/#es-USVString, https://heycam.github.io/webidl/#es-DOMString & https://tc39.es/ecma262/#sec-tostring). TypeScript typings do not usually reflect the coercible types, only the strict types. The strict type for the record value here is the string, thus typings for argument 1 of URLSearchParams should be string[][] | Record<string, string> | string (or [string, string][] | Record<string, string> | string).

If typings should also represent coercion states, we could type essentially all Web APIs as taking any because WebIDL is very lax 😉. Fun fact: for WebIDL the only invalid URLSearchParams constructor is new URLSearchParams(Symbol()), or other object or iterable types containing symbols.

@jimmywarting
Copy link

jimmywarting commented Jun 27, 2021

There are many valid DOM Api's that takes "any" as a valid argument and it bother me on so many levels that TypeScript treats it so strictly. Like: it should be totally fine to do: new Blob([{}]) if you want to for some reason or doing fetch(new URL(...)), new ArrayBuffer("23") and even omitting arguments in ArrayBuffer to allow for new ArrayBuffer()

Take this bit mask example that takes a bunch o boolean and turn it into a number

function createMask (...args: boolean[]) {
    var nMask = 0, nFlag = 0, nLen = args.length
    for (nFlag; nFlag < nLen; nMask |= args[nFlag] << nFlag++)
    return nMask
}

const isLoggedIn = true
const hasReadPermission = true
const hasWritePermission = false

createMask(isLoggedIn, hasReadPermission, hasWritePermission) // 3

TS is complaining that nFlag isn't a number, but it's totally fine to do bitwise operators on boolean to, cuz in the end boolean is just a 1 or a 0

The WebIDL dose so many things to converting xyz to the correct type that it needs. I think TS needs to better understand symbol.toPrimitive, toString and toJSON stuff and how webidl conversion works

@lucacasonato
Copy link

lucacasonato commented Jun 27, 2021

@jimmywarting Typescript treats USVString as a string because it is a string. It also treats ByteString or DOMString as a string, because they are also strings. There is nothing special about USVString in regards to type coercion. All types except Symbol can be coerced into USVString/DOMString/ByteString in WebIDL. I don't think typescript should represent this though, as the point of it is to prevent runtime coercion. new Blob([{}]) resulting in a blob with contents [object Object] is not something typescript should cater to IMO. It defeats the purpose of strict types.

So I don't think you agree with me, because I think this issue is invalid and should be closed as working as intended 😄

@ekwoka
Copy link

ekwoka commented Aug 24, 2022

new Blob([{}]) resulting in a blob with contents [object Object] is not something typescript should cater to IMO. It defeats the purpose of strict types.

Little bit of a useless example, since nobody here is trying to make the argument it should accept any. Just that it can accept the core primitives as there are no similar gotchas like you're describing.

@JGJP
Copy link

JGJP commented Sep 30, 2022

Can we bring back this discussion? I think it's enough for it to accept primitives that we know can be converted to strings, and also any object that implements the toString() method with a string return type

@Kris-Pelteshki
Copy link

I can't believe the TS team seriously went with this decision.
As an enthusiastic TS strict user, I would much rather do new URLSearchParams(({take: 15, color: false}) as any)
Instead of doing things like false.ToString()

@spamshaker
Copy link

spamshaker commented Jun 12, 2023

really confusing, may we get it?
microsoft/TypeScript#54621

@DanielRosenwasser
Copy link
Member

The way to fix this is actually to update TypeScript-DOM-lib-generator with overriding types.

Here's a test case of what I'd expect to work:

export const carQuery = new URLSearchParams([
    ["query", "suv"],
    ["new", true],
    ["accidents", false],
    ["miles", 42_000],
]);

carQuery.set("used", true);
carQuery.append("year", 2023);
carQuery.append("year", 2024);

let str: string | null, strs: string[];

str = carQuery.get("query");
strs = carQuery.getAll("year");

for (const [key, value] of carQuery) {
    str = key;
    str = value;
}
for (const [key, value] of carQuery.entries()) {
    str = key;
    str = value;
}
for (const value of carQuery.values()) {
    str = value;
}
carQuery.forEach((value, key) => {
    str = key;
    str = value;
});

@DanielRosenwasser DanielRosenwasser transferred this issue from microsoft/TypeScript Jun 12, 2023
@lucacasonato
Copy link

@DanielRosenwasser what is the general policy for when lib files should encode coercion into types?

I really think allowing primitive coercion here is confusing. One may think that the value true is represented as "on" in the query parameters, because that's what checked=true encodes as in HTML forms encoding to query params.

Numbers are iffy, but Booleans seem just outright confusing.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jun 12, 2023

It is usually driven by practical usage in JS and what the tradeoffs in safety are - usually on a case-by-case basis.

Honestly, I'm kind of surprised that booleans are confusing here. I can see that checked has that weirdness, but if I got an error that true was not allowed, I wouldn't think to myself "oh, I should have written the string "on"". Instead I would write the string "true" because I'd assume that the types permitted are too strict.

@lucacasonato
Copy link

I think true and false can have many other representations too. For example, in query strings these are all things that people use to represent true of the key x

?x
?x=
?x=true
?x=on

and false

?
?x=false

As a user I may well suspect either true or false encode to one of these other representations.

I can get behind numbers, because they are unambiguously encoded - but true and false... it has the possibility for weird confusion.

I've often seen folks do:

if (query.used) searchParams.set("used", "");

As a user not super in my depth I could get confused and think that the initializer form of true / false may be a shorthand for this.

What about strings and numbers only?

@DanielRosenwasser
Copy link
Member

Strings and numbers only is what @RyanCavanaugh recently recommended for a PR attempt, so I'm okay with that as a first step.

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

Successfully merging a pull request may close this issue.