[node] improve typing of query.parse() #22171
Conversation
|
types/node/index.d.ts to authors (@DefinitelyTyped/DefinitelyTyped @parambirs @tellnes @WilcoBakker @octo-sniffle @smac89 @mwiktorczyk @wwwy3y3 @DeividasBakanas @kjin @alvis @OliverJAsh @eps1lon @Hannes-Magnusson-CK @jkomyno Microsoft TypeScript (account can't be detected)). Could you review this PR? |
|
A definition author 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! |
types/node/index.d.ts
Outdated
| query: ParsedUrlQuery; | ||
| } | ||
|
|
||
| export interface UrlStringQuery extends Url { |
There was a problem hiding this comment.
Perhaps rename these types to UrlWith{StringQuery,ParsedQuery} for readability?
| // Output of `url.parse` | ||
| export interface Url extends UrlObjectCommon { | ||
| port?: string; | ||
| query?: string | null | ParsedUrlQuery; |
There was a problem hiding this comment.
Maybe remove this, and make the boolean parse overload return UrlParsedQuery | UrlStringQuery?
There was a problem hiding this comment.
seems Url.url is used at several other modules so I will keep it.
- Better naming UrlParsedQuery => UrlWithParsedQuery, UrlStringQuery => UrlWithStringQuery - replace Url type by UrlWithParsedQuery | UrlWithStringQuery
|
Thanks for the review. Pushed an update. |
|
@Flarna 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! |
|
CI fails unrelated, will try to fix via another PR but it seems fails are introduced faster then I can fix them... |
* tabs to spaces * [node] improve typing of query.parse() * Update according to review: - Better naming UrlParsedQuery => UrlWithParsedQuery, UrlStringQuery => UrlWithStringQuery - replace Url type by UrlWithParsedQuery | UrlWithStringQuery * Re-introduce url.Url as it is used by several other modules.
npm run lint package-name(ortscif notslint.jsonis present).If changing an existing definition:
tslint.jsoncontaining{ "extends": "dtslint/dt.json" }.Followup on #20720. Try to further improve typing of
url.parse()and to avoid reported problems.@connor4312 @bowdenk7 Please take a look, try in your code and let us know the results. Thanks!