[@types/urijs] Fix unnecessarily strict query params

[binury]

• 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.
• Follow the advice from the readme.
• Avoid common mistakes.
• Run npm test <package to test>.

changing an existing definition:

• Provide a URL to documentation or source code which provides context for the suggested changes: https://github.com/medialize/URI.js/blob/797789f8b77aea63886d203a0f2b98e7374fc521/src/URI.js#L750-L778
• If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.

[typescript-bot]

@binury Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through.

This is a live comment which I will keep updated.

1 package in this PR

• urijs — on npm, on unpkg
  • owner-approval: @marcelltoth

Code Reviews

Because you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it.

You can test the changes of this PR in the Playground.

Status

• ✅ No merge conflicts
• ✅ Continuous integration tests have passed
• ✅ Most recent commit is approved by type definition owners or DT maintainers

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.

---

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 54650,
  "author": "binury",
  "headCommitOid": "ee7d02a8ec3e4f0b3e32277049bd93e93f94c4b6",
  "lastPushDate": "2021-09-05T16:18:12.000Z",
  "lastActivityDate": "2021-09-07T18:29:26.000Z",
  "mergeOfferDate": "2021-09-06T14:20:27.000Z",
  "mergeRequestDate": "2021-09-07T18:29:26.000Z",
  "mergeRequestUser": "binury",
  "hasMergeConflict": false,
  "isFirstContribution": true,
  "tooManyFiles": false,
  "popularityLevel": "Popular",
  "pkgInfo": [
    {
      "name": "urijs",
      "kind": "edit",
      "files": [
        {
          "path": "types/urijs/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/urijs/test/urijs-dom.test.ts",
          "kind": "test"
        },
        {
          "path": "types/urijs/test/urijs-nodejs.test.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "RodneyJT",
        "xt0rted",
        "petejohanson",
        "ljqx",
        "teamworkguy2",
        "Akuukis",
        "marcelltoth",
        "XVincentX",
        "ahagelstein",
        "apepper"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Popular"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "marcelltoth",
      "date": "2021-09-06T14:19:50.000Z",
      "isMaintainer": false
    }
  ],
  "mainBotCommentID": 883670372,
  "ciResult": "pass"
}

[typescript-bot]

🔔 @RodneyJT @xt0rted @petejohanson @ljqx @TeamworkGuy2 @Akuukis @marcelltoth @XVincentX @ahagelstein @apepper — 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.

[binury]

For some reason, URI query params were declared as strictly as string|null|string[]|null. Initially I just wanted to amend this to include booleans and numbers but as I tested changes and dug through the source, I remembered awhile back encountering an issue when a param value was NaN and I felt it would make more sense here to open up the params as any instead of trying to list every possible "sensible" type like boolean | number | string | null | undefined | Array<boolean | number | string | null | undefined>

[typescript-bot]

Re-ping @RodneyJT, @xt0rted, @petejohanson, @ljqx, @TeamworkGuy2, @Akuukis, @marcelltoth, @XVincentX, @ahagelstein, @apepper:

This PR has been out for over a week, yet I haven't seen any reviews.

Could someone please give it some attention? Thanks!

[marcelltoth]

This is likely less safe than before. any can take literally anything, including objects, Maps, and a lot of things we shouldn't allow. What's wrong with taking your list (assuming you double checked it is correct): boolean | number | string | null | undefined | Array<boolean | number | string | null | undefined>.

Also to be correct we should make it Partial<Record<....>> because of course it is a partial. (Non-partial Record<string>s almost never make any sense.)

[binury]

QueryDataMaps can take literally anything including objects, Maps, and a lot of things

[binury]

Non-partial Records almost never make any sense.

These are parameters for a URL

[binury]

What's wrong with taking your list?
<boolean | number | string | null | undefined | Array<boolean | number | string | null | undefined>

The issue that I see is that it would be incomplete, because urijs queries can support any object, even if it seems like it shouldn't. https://github.com/medialize/URI.js/blob/gh-pages/src/URI.js#L782-L811

 map = new Map()
 map.set('a', 'b')
 uri('github.com').addQuery({'map':map}).toString() // => 'github.com?map=%5Bobject+Map%5D'

[typescript-bot]

@binury One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you!

[binury]

@marcelltoth

What's wrong with taking your list?
<boolean | number | string | null | undefined | Array<boolean | number | string | null | undefined>

The issue that I see is that it would be incomplete, because urijs queries can support any object, even if it seems like it shouldn't. https://github.com/medialize/URI.js/blob/gh-pages/src/URI.js#L782-L811

 map = new Map()
 map.set('a', 'b')
 uri('github.com').addQuery({'map':map}).toString() // => 'github.com?map=%5Bobject+Map%5D'

[binury]

A similar debate has been addressed in the Typescript repo:

• Update lib.d.ts: Change the type of uriComponent (passed to encodeURIComponent) from string to string | number | boolean microsoft/TypeScript#31103
• encodeURIComponent should accept any value type microsoft/TypeScript#18159

This is the function that urijs is using calling to parse queries and its flexibility is why I opted to allow any

However, maybe following precedent would be the sensible route –in lieu of consensus?– which was to "[represent] the intended use cases".

[typescript-bot]

@marcelltoth Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

[marcelltoth]

I see your point. Actually you convinced me, I'm good with making it any, but please make it a Partial (unless you see a reason not to), then I'll approve.

[binury]

I see your point. Actually you convinced me, I'm good with making it any, but please make it a Partial (unless you see a reason not to), then I'll approve.

Sorry, I'm not familiar with TS partials— a partial of ?

[typescript-bot]

Re-ping @RodneyJT, @xt0rted, @petejohanson, @ljqx, @TeamworkGuy2, @Akuukis, @marcelltoth, @XVincentX, @ahagelstein, @apepper:

This PR has been out for over a week, yet I haven't seen any reviews.

Could someone please give it some attention? Thanks!

[typescript-bot]

It has been more than two weeks and this PR still has no reviews.

I'll bump it to the DT maintainer queue. Thank you for your patience, @binury.

(Ping @RodneyJT, @xt0rted, @petejohanson, @ljqx, @TeamworkGuy2, @Akuukis, @marcelltoth, @XVincentX, @ahagelstein, @apepper.)

[TeamworkGuy2]

I think @marcelltoth was referring to the type QueryDataMap = Record<string, any>.
A partial is this https://github.com/microsoft/TypeScript/blob/8894262baccb1d965e71a2d7830236b431e224f7/lib/lib.es5.d.ts#L1471
So I think he's saying make type QueryDataMap equal Partial<Record<string, any>>

[typescript-bot]

@marcelltoth Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

[typescript-bot]

@binury: Everything looks good here. I am ready to merge this PR (at ee7d02a) on your behalf whenever you think it's ready.

If you'd like that to happen, please post a comment saying:

Ready to merge

and I'll merge this PR almost instantly. Thanks for helping out! ❤️

(@RodneyJT, @xt0rted, @petejohanson, @ljqx, @TeamworkGuy2, @Akuukis, @marcelltoth, @XVincentX, @ahagelstein, @apepper: you can do this too.)

[binury]

Ready to merge

[typescript-bot]

I just published @types/urijs@1.19.17 to npm.