[node] improve typing of query.parse() by Flarna · Pull Request #22171 · DefinitelyTyped/DefinitelyTyped

Flarna · 2017-12-13T21:57:36Z

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.)
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://nodejs.org/dist/latest-v9.x/docs/api/url.html#url_url_parse_urlstring_parsequerystring_slashesdenotehost
Increase the version number in the header if appropriate.
If you are making substantial changes, consider adding a tslint.json containing { "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!

dt-bot · 2017-12-13T21:57:38Z

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?
👍 or 👎?

typescript-bot · 2017-12-16T22:13:58Z

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!

OliverJAsh · 2017-12-18T10:01:16Z

+        query: ParsedUrlQuery;
+    }
+
+    export interface UrlStringQuery extends Url {


Perhaps rename these types to UrlWith{StringQuery,ParsedQuery} for readability?

OliverJAsh · 2017-12-18T10:02:36Z

@@ -2342,7 +2342,19 @@ declare module "url" {
        query?: string | null | ParsedUrlQuery;


Maybe remove this, and make the boolean parse overload return UrlParsedQuery | UrlStringQuery?

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

Flarna · 2017-12-18T10:27:44Z

Thanks for the review. Pushed an update.

typescript-bot · 2017-12-18T17:00:56Z

@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!

Flarna · 2018-01-03T09:56:26Z

CI fails unrelated, will try to fix via another PR but it seems fails are introduced faster then I can fix them...

Flarna · 2018-01-03T19:02:35Z

@mhegazy, @Andy-MS restify fail is unrelated, created #22653 to correct it.
Any chance to get this merged before again some unrelated change breaks build?

* 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.

Flarna added 2 commits December 13, 2017 21:15

tabs to spaces

6354056

[node] improve typing of query.parse()

ec059c9

Flarna mentioned this pull request Dec 13, 2017

Node: remove redundant widen of query type to any + tidy up #20720

Merged

8 tasks

typescript-bot added the Popular package This PR affects a popular package (as counted by NPM download counts). label Dec 13, 2017

connor4312 approved these changes Dec 14, 2017

View reviewed changes

typescript-bot added the Other Approved This PR was reviewed and signed-off by a community member. label Dec 14, 2017

typescript-bot added the Merge:LGTM label Dec 16, 2017

OliverJAsh approved these changes Dec 18, 2017

View reviewed changes

Update according to review:

94c5552

- Better naming UrlParsedQuery => UrlWithParsedQuery, UrlStringQuery => UrlWithStringQuery - replace Url type by UrlWithParsedQuery | UrlWithStringQuery

typescript-bot added Author Approved and removed Merge:LGTM labels Dec 18, 2017

OliverJAsh approved these changes Dec 18, 2017

View reviewed changes

Re-introduce url.Url as it is used by several other modules.

55d6dda

typescript-bot added The Travis CI build failed and removed Merge:Express labels Dec 18, 2017

Flarna added 2 commits December 22, 2017 17:03

Merge remote-tracking branch 'upstream/master' into node_url_parse

64f4d6d

Merge remote-tracking branch 'upstream/master' into node_url_parse

5a5c0e1

typescript-bot added The Travis CI build failed and removed The Travis CI build failed labels Dec 29, 2017

Flarna added 2 commits January 2, 2018 19:45

Merge remote-tracking branch 'upstream/master' into node_url_parse

c23b1e8

Merge remote-tracking branch 'upstream/master' into node_url_parse

8477dec

typescript-bot added The Travis CI build failed and removed The Travis CI build failed labels Jan 2, 2018

Merge remote-tracking branch 'upstream/master' into node_url_parse

b062d5e

typescript-bot removed the The Travis CI build failed label Jan 2, 2018

Merge remote-tracking branch 'upstream/master' into node_url_parse

6bed328

typescript-bot added the The Travis CI build failed label Jan 3, 2018

Merge remote-tracking branch 'upstream/master' into node_url_parse

94f56e1

typescript-bot removed the The Travis CI build failed label Jan 3, 2018

typescript-bot added the The Travis CI build failed label Jan 3, 2018

Flarna mentioned this pull request Jan 3, 2018

[restify] Fix build by removing wrong headers property from Response #22653

Merged

8 tasks

Merge remote-tracking branch 'upstream/master' into node_url_parse

c6ce392

typescript-bot removed the The Travis CI build failed label Jan 3, 2018

ghost merged commit d9f51ea into DefinitelyTyped:master Jan 3, 2018

Flarna deleted the node_url_parse branch January 3, 2018 20:20

Flarna mentioned this pull request Jan 10, 2018

[node] sync nodejs v8 with recent changes done on latest #22816

Merged

8 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[node] improve typing of query.parse()#22171

[node] improve typing of query.parse()#22171
12 commits merged intoDefinitelyTyped:masterfrom
Flarna:node_url_parse

Flarna commented Dec 13, 2017

Uh oh!

dt-bot commented Dec 13, 2017

Uh oh!

typescript-bot commented Dec 16, 2017 •

edited

Loading

Uh oh!

OliverJAsh Dec 18, 2017

Uh oh!

OliverJAsh Dec 18, 2017

Uh oh!

Flarna Dec 18, 2017

Uh oh!

Flarna commented Dec 18, 2017

Uh oh!

typescript-bot commented Dec 18, 2017

Uh oh!

Flarna commented Jan 3, 2018

Uh oh!

Flarna commented Jan 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		@@ -2342,7 +2342,19 @@ declare module "url" {
		query?: string \| null \| ParsedUrlQuery;

Conversation

Flarna commented Dec 13, 2017

Uh oh!

dt-bot commented Dec 13, 2017

Uh oh!

typescript-bot commented Dec 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OliverJAsh Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

OliverJAsh Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

Flarna Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

Flarna commented Dec 18, 2017

Uh oh!

typescript-bot commented Dec 18, 2017

Uh oh!

Flarna commented Jan 3, 2018

Uh oh!

Flarna commented Jan 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

typescript-bot commented Dec 16, 2017 •

edited

Loading