Skip to content

Conversation

@OliverJAsh
Copy link
Contributor

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.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: Fix node url and http/https request types #18766 (comment)
  • 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" }.

@dt-bot
Copy link
Member

dt-bot commented Oct 19, 2017

types/node/index.d.ts

to authors (@DefinitelyTyped/DefinitelyTyped @parambirs @tellnes @WilcoBakker @octo-sniffle @smac89 @Flarna @mwiktorczyk @wwwy3y3 @DeividasBakanas @kjin @alvis Microsoft TypeScript (account can't be detected)). Could you review this PR?
👍 or 👎?

export interface Url extends UrlObject {
export interface OutputUrlObject extends InputUrlObject {
port?: string;
query?: any;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already provided and typed correctly in UrlObject (now InputUrlObject)

export function parse(urlStr: string, parseQueryString?: boolean, slashesDenoteHost?: boolean): OutputUrlObject;
export function format(URL: URL, options?: URLFormatOptions): string;
export function format(urlObject: UrlObject | string): string;
export function format(urlObject: InputUrlObject | string): string;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For clarification on "input" and "output" types: #18766 (comment)

@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). The Travis CI build failed labels Oct 19, 2017
@typescript-bot
Copy link
Contributor

@OliverJAsh Please fix the failures indicated in the Travis CI log.

@OliverJAsh
Copy link
Contributor Author

Travis build failed due to timeout, but there were no errors, so this should be good.

Copy link
Contributor

@Flarna Flarna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using ParsedUrlQuery is fine, but is it really needed to rename the existing interfaces?
This may break builds; actually CI shows quite some fails because of this.

@OliverJAsh
Copy link
Contributor Author

This may break builds; actually CI shows quite some fails because of this.

Thanks, will revert!

@OliverJAsh
Copy link
Contributor Author

@Flarna Reverted now

slashes?: boolean;
}

export interface Url extends UrlObject {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a look into the docu and I have the feeling some widening is needed for Url:
https://nodejs.org/dist/latest-v8.x/docs/api/url.html#url_url_format_urlobject tells that querystring#stringify is used by url#format.
https://nodejs.org/dist/latest-v8.x/docs/api/querystring.html#querystring_querystring_stringify_obj_sep_eq_options tells It serializes the following types of values passed in obj: <string> | <number> | <boolean> | <string[]> | <number[]> | <boolean[]> Any other input values will be coerced to empty strings.
Not sure if the Any other input values will be coerced to empty strings. part is really needed by anyone as and it may hide real issues but boolean and number (value and Array) sounds reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, query: any makes sense as the input for url.format, but it's currently used as the output of url.parse. Should we swap them?

E.g.

// this represents the input of url.format
interface UrlObject {
  query?: any;
}

// this represents the output of url.parse
interface Url extends UrlObject {
  query: string | null | ParsedUrlQuery;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not go that far as e.a. a single RegEx/Array are not allowed. What about following as input for format:

interface Url extends UrlObject {
  // other fields
  query?: string | null | { [key: string]: undefined | null | boolean | number | string | Array<boolean | number | string> }
}

I know that this does not allow any type in the properties as it is in javascript. But as indicated above I would argue that such use of the api is questionable and at least I prefer that typescript tells me about this and I have to add a cast or toString().
The more relaxed variant would be following which is still better then a plain any.

interface Url extends UrlObject {
  // other fields
  query?: string | null | { [key: string]: any }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, however Url is currently the output of parse, not the input of format. Are you suggesting swapping them?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems I did a copy paste issue here sorry.
I don't think we should swap them as this would be a similar action as renaming which has shown to cause side effects to other modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the sounds of it, Url is supposed to represent the input of format. As you have pointed out, it accepts wider types than UrlObject because, if provided as arguments to format, Node will coerce them. Do I understand correctly? If this is the case, it seems an error to me that Url is currently used as the output of parse and UrlObject is used as the input of format. When parsing a query string, I do not want TypeScript to think my query string values are any!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know the full history of node typings as I don't use it that long. Current state is that url.format() gets an UrlObject and url.parse() returns an Url. Changing the names which would be nice has bad side effects so I don't think we should do this now.

format accepts wider types then parse returns (at least for port and query). I'm not really a fan of expressing this by extending an interface - but it is like it is - don't know why.

What about creating three types: UrlObjectCommon holding the comon part, UrlObject extends it by adding the properties special to format and Url extends it by adding the properties special to parse. This would be still backward compatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestions! I've just pushed some changes, see what you think? @Flarna

@RyanCavanaugh
Copy link
Member

@OliverJAsh Please fix the failures indicated in the Travis CI log.

@OliverJAsh
Copy link
Contributor Author

@RyanCavanaugh As with all my other Node typing related PRs, the build failed due to a timeout. This seems to an issue when linting Node typings, not related to any of my changes.

The job exceeded the maximum time limit for jobs, and has been terminated.

@Flarna Flarna mentioned this pull request Nov 5, 2017
@OliverJAsh
Copy link
Contributor Author

Ping @RyanCavanaugh @Flarna. Build failed due to timeout. Please can we merge?

@OliverJAsh OliverJAsh closed this Nov 10, 2017
@OliverJAsh OliverJAsh reopened this Nov 10, 2017
@OliverJAsh OliverJAsh closed this Dec 3, 2017
@OliverJAsh OliverJAsh reopened this Dec 3, 2017
@Flarna
Copy link
Contributor

Flarna commented Dec 3, 2017

I think commit ec9120e (see #21897) has the fix for your failed CI run.

@typescript-bot
Copy link
Contributor

Approved by a listed owner. PR ready to merge pending express review by a maintainer.

@bowdenk7 bowdenk7 merged commit ee2ab18 into DefinitelyTyped:master Dec 6, 2017
@connor4312
Copy link
Contributor

connor4312 commented Dec 13, 2017

This change is very heavily breaking for many consumers. There's a very good chance anyone (like us!) who did things with url.query had their builds fail.

@bowdenk7
Copy link

is the submitted type for url.query wrong?

@Flarna
Copy link
Contributor

Flarna commented Dec 13, 2017

I think it would be better to create a dedicated issue to discuss this further and please provide some more details what exactly is broken and why you think that the type definition is wrong.

url.query was any before now it is string | null | ParsedUrlQuery which is more concrete and as a result it allows typescript to do type checking now. So if this breaks code it's likely that it discovered a possible issue. if this is not wanted it's maybe better to stick on plain Javascript 😉.

Type is correct to my understanding (see https://nodejs.org/dist/latest-v9.x/docs/api/url.html#url_urlobject_query).

But I think url.parse() could be further improved to return string | null for the case where parseQueryString is not given or false and ParsedUrlQuery if it is true.

@connor4312
Copy link
Contributor

connor4312 commented Dec 13, 2017

Url objects are passed through in very many places in the Node API and various library APIs, particularly around incoming HTTP requests. In our particular use case we know that the URL query string on an incoming request is parsed to an object. Now, all cases where we used the query string broke, since those query string properties aren't accessible on the string type. This is not a 'possible issue' in our code, as this is code that's been in production for years without any issues. It's an issue in the typings.

If the concern is just around url.parse, I'd suggest a function overload for that, leaving the original interface alone.

@OliverJAsh
Copy link
Contributor Author

OliverJAsh commented Dec 13, 2017 via email

@Flarna
Copy link
Contributor

Flarna commented Dec 13, 2017

to be continued at #22171

KSXGitHub pushed a commit to KSXGitHub/DefinitelyTyped that referenced this pull request May 12, 2018
Node: remove redundant widen of query type to any + tidy up
@OliverJAsh OliverJAsh deleted the patch-14 branch February 25, 2019 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Popular package This PR affects a popular package (as counted by NPM download counts).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants