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-string] Make types more accurate #29395

Merged
merged 6 commits into from Oct 10, 2018

Conversation

Projects
None yet
5 participants
@szhu
Contributor

szhu commented Oct 2, 2018

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: https://www.npmjs.com/package/query-string
  • Increase the version number in the header if appropriate. Do I need to?
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.
@typescript-bot

This comment has been minimized.

Show comment
Hide comment
@typescript-bot

typescript-bot Oct 2, 2018

@szhu Thank you for submitting this PR!

🔔 @SamVerschueren @tkrotoff @huhuanming @MadaraUchiha @shssoichiro - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

typescript-bot commented Oct 2, 2018

@szhu Thank you for submitting this PR!

🔔 @SamVerschueren @tkrotoff @huhuanming @MadaraUchiha @shssoichiro - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@typescript-bot typescript-bot moved this from Waiting for Reviewers to Needs Author Attention in Pull Request Status Board Oct 3, 2018

@typescript-bot

This comment has been minimized.

Show comment
Hide comment
@typescript-bot

typescript-bot Oct 3, 2018

@szhu The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

typescript-bot commented Oct 3, 2018

@szhu The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@typescript-bot

This comment has been minimized.

Show comment
Hide comment
@typescript-bot

typescript-bot Oct 3, 2018

@szhu The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

typescript-bot commented Oct 3, 2018

@szhu The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@typescript-bot typescript-bot moved this from Needs Author Attention to Waiting for Reviewers in Pull Request Status Board Oct 5, 2018

@MadaraUchiha

This comment has been minimized.

Show comment
Hide comment
@MadaraUchiha

MadaraUchiha Oct 5, 2018

Contributor

Your change is incorrect.

According to the README listed on the package, an array may be returned as well:

queryString.parse('foo[]=1&foo[]=2&foo[]=3', {arrayFormat: 'bracket'});
//=> foo: [1,2,3]

I believe this is why it was set to be any to begin with.

Contributor

MadaraUchiha commented Oct 5, 2018

Your change is incorrect.

According to the README listed on the package, an array may be returned as well:

queryString.parse('foo[]=1&foo[]=2&foo[]=3', {arrayFormat: 'bracket'});
//=> foo: [1,2,3]

I believe this is why it was set to be any to begin with.

@szhu

This comment has been minimized.

Show comment
Hide comment
@szhu

szhu Oct 9, 2018

Contributor

Fixed the types!

  • I updated the test format to be a little more useful; ie will prevent erroneous changes like my previous one. I don't think the test from before was really catching much.
  • Interesting discovery: query-string supports only a subset of the conventional array expressions, no wonder this package is relatively unmaintained. It looks like https://www.npmjs.com/package/qs is the de facto package for doing query string parsing.
Contributor

szhu commented Oct 9, 2018

Fixed the types!

  • I updated the test format to be a little more useful; ie will prevent erroneous changes like my previous one. I don't think the test from before was really catching much.
  • Interesting discovery: query-string supports only a subset of the conventional array expressions, no wonder this package is relatively unmaintained. It looks like https://www.npmjs.com/package/qs is the de facto package for doing query string parsing.

@typescript-bot typescript-bot moved this from Waiting for Reviewers to Needs Author Attention in Pull Request Status Board Oct 9, 2018

@typescript-bot

This comment has been minimized.

Show comment
Hide comment
@typescript-bot

typescript-bot Oct 9, 2018

@szhu The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

typescript-bot commented Oct 9, 2018

@szhu The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@typescript-bot typescript-bot moved this from Needs Author Attention to Waiting for Reviewers in Pull Request Status Board Oct 9, 2018

}
export interface OutputParams {
[key: string]: string | string[] | undefined;

This comment has been minimized.

@shssoichiro

shssoichiro Oct 10, 2018

Contributor

What is the use case for the undefined possibility? Is this to catch e.g. an empty query string? I'm not sure it's necessary to have that, but it may be worth adding a test case to validate this use case.

@shssoichiro

shssoichiro Oct 10, 2018

Contributor

What is the use case for the undefined possibility? Is this to catch e.g. an empty query string? I'm not sure it's necessary to have that, but it may be worth adding a test case to validate this use case.

This comment has been minimized.

@szhu

szhu Oct 10, 2018

Contributor

This gets around what I feel like is an inconsistency in TypeScript. Here's an illustration:

The object {a: 'hi'} fits into the type {[key: string}: string}. However that type represents "an object whose values at all string indices is a string". This is not true -- only the value at 'a' is a string. The returned value at all other indices is undefined.

This is something that TypeScript thinks is correct but will error at runtime:

let obj: {[key: string}: string} = {a: 'hi'};
obj['b'].length // Uncaught TypeError: Cannot read property 'length' of undefined

If we instead used the type {[key: string}: string | undefined}, we will get a static error for the above.

@szhu

szhu Oct 10, 2018

Contributor

This gets around what I feel like is an inconsistency in TypeScript. Here's an illustration:

The object {a: 'hi'} fits into the type {[key: string}: string}. However that type represents "an object whose values at all string indices is a string". This is not true -- only the value at 'a' is a string. The returned value at all other indices is undefined.

This is something that TypeScript thinks is correct but will error at runtime:

let obj: {[key: string}: string} = {a: 'hi'};
obj['b'].length // Uncaught TypeError: Cannot read property 'length' of undefined

If we instead used the type {[key: string}: string | undefined}, we will get a static error for the above.

This comment has been minimized.

@szhu

szhu Oct 10, 2018

Contributor

Here's a more relevant example:

import queryString from "query-string";
let params = queryString.parse("a=1");
params['b'].length

This will error at runtime without | undefined. It will be a static type error with | undefined.

@szhu

szhu Oct 10, 2018

Contributor

Here's a more relevant example:

import queryString from "query-string";
let params = queryString.parse("a=1");
params['b'].length

This will error at runtime without | undefined. It will be a static type error with | undefined.

This comment has been minimized.

@shssoichiro

shssoichiro Oct 10, 2018

Contributor

Interesting. Makes sense to me, thanks.

@shssoichiro

shssoichiro Oct 10, 2018

Contributor

Interesting. Makes sense to me, thanks.

@typescript-bot typescript-bot moved this from Waiting for Reviewers to Check and Merge in Pull Request Status Board Oct 10, 2018

@typescript-bot

This comment has been minimized.

Show comment
Hide comment
@typescript-bot

typescript-bot Oct 10, 2018

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

typescript-bot commented Oct 10, 2018

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

@@ -19,20 +19,26 @@ import * as queryString from 'query-string';
result = queryString.stringify({ foo: 'bar' }, { strict: false, encode: false });
}
// For each section below, the second line ensures the real answer is of the declared

This comment has been minimized.

@andy-ms

andy-ms Oct 10, 2018

Contributor

You can also use // $ExpectType. https://github.com/Microsoft/dtslint#write-tests

@andy-ms

andy-ms Oct 10, 2018

Contributor

You can also use // $ExpectType. https://github.com/Microsoft/dtslint#write-tests

This comment has been minimized.

@szhu

szhu Oct 10, 2018

Contributor

Ooh, didn't know about that. $ExpectType won't work for the tests below though, right?

@szhu

szhu Oct 10, 2018

Contributor

Ooh, didn't know about that. $ExpectType won't work for the tests below though, right?

This comment has been minimized.

@andy-ms

andy-ms Oct 10, 2018

Contributor

In all the below tests the output is of type queryString.OutputParams, so it looks like they're just testing that certain things are assignable to that? Then yeah, // $ExpectType wouldn't work well.

@andy-ms

andy-ms Oct 10, 2018

Contributor

In all the below tests the output is of type queryString.OutputParams, so it looks like they're just testing that certain things are assignable to that? Then yeah, // $ExpectType wouldn't work well.

This comment has been minimized.

@szhu

szhu Oct 10, 2018

Contributor

Got it. So to confirm, a test could be written to be this?

    // queryString.parse('?foo=bar');
    // $ExpectType queryString.OutputParams
    {foo: "bar"};
@szhu

szhu Oct 10, 2018

Contributor

Got it. So to confirm, a test could be written to be this?

    // queryString.parse('?foo=bar');
    // $ExpectType queryString.OutputParams
    {foo: "bar"};

This comment has been minimized.

@andy-ms

andy-ms Oct 10, 2018

Contributor

No, because // $ExpectType just matches the stringified type with the expected string and doesn't consider subtyping. (The type of {foo: "bar"} is { foo: string }.) For considering assignability of an object literal to a named type what you're doing now is fine.

@andy-ms

andy-ms Oct 10, 2018

Contributor

No, because // $ExpectType just matches the stringified type with the expected string and doesn't consider subtyping. (The type of {foo: "bar"} is { foo: string }.) For considering assignability of an object literal to a named type what you're doing now is fine.

This comment has been minimized.

@szhu

szhu Oct 10, 2018

Contributor

Got it, thanks for clarifying!

@szhu

szhu Oct 10, 2018

Contributor

Got it, thanks for clarifying!

@andy-ms andy-ms merged commit e174f81 into DefinitelyTyped:master Oct 10, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@typescript-bot typescript-bot removed this from Check and Merge in Pull Request Status Board Oct 10, 2018

@szhu szhu deleted the szhu:patch-2 branch Oct 10, 2018

YashdalfTheGray added a commit to YashdalfTheGray/DefinitelyTyped that referenced this pull request Oct 15, 2018

[query-string] Make types more accurate (DefinitelyTyped#29395)
* [query-string] Make types more accurate

* Update index.d.ts

* Fix ts errors

* Fix linter errors

* Fix types and make tests more accurate

* Fix linter errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment