Fix node url and http/https request types#18766
Fix node url and http/https request types#187666 commits merged intoDefinitelyTyped:masterfrom jeffkenney:fix-node-url-types
Conversation
|
types/node/v6/index.d.ts to authors (@DefinitelyTyped/DefinitelyTyped @WilcoBakker Microsoft TypeScript (account can't be detected)). Could you review this PR? Checklist
|
| export interface Server extends tls.Server { } | ||
| export function createServer(options: ServerOptions, requestListener?: Function): Server; | ||
| export function request(options: RequestOptions, callback?: (res: http.IncomingMessage) => void): http.ClientRequest; | ||
| export function request(options: RequestOptions | string, callback?: (res: http.IncomingMessage) => void): http.ClientRequest; |
There was a problem hiding this comment.
why is not this change made to other versions of node, i.e. v7, and v8?
|
Hey @jeffkenney could you apply the change to the other versions as well? |
typescript-bot
added
the
Unmerged
|
This PR has been open and unchanged 5 days without signoff or complaint. This will be merged by a maintainer soon if there are no objections. |
|
@mhegazy @DanielRosenwasser I'll work on getting the other versions updated. |
bowdenk7
added
the
Revision needed
|
Will merge in once the other version is updated. Thanks! |
|
Updates applied to all versions, also backported the split Url / UrlObject types to v0 and v4. The CI failures look unrelated. |
typescript-bot
added
The Travis CI build failed
and removed
Unmerged
|
@jeffkenney Please fix the failures indicated in the Travis CI log. |
typescript-bot
added
Abandoned
typescript-bot
added
the
Popular package
typescript-bot
removed
the
Revision needed
|
Hi @jeffkenney, could you fix the merge conflict? Then I'll take a quick look at the CI and hopefully merge. Thanks! |
ghost
merged commit 
| export function parse(urlStr: string, parseQueryString?: boolean, slashesDenoteHost?: boolean): Url; | ||
| export function format(URL: URL, options?: URLFormatOptions): string; | ||
| export function format(urlObject: UrlObject): string; | ||
| export function format(urlObject: UrlObject | string): string; |
There was a problem hiding this comment.
Doesn't format use the same type as parse returns? If so, how come we need both UrlObject and Url? Won't the latter suffice?
Also, I would suggest renaming Url to ParsedURL to disambiguate it from URL
There was a problem hiding this comment.
From node docu: The value of urlObject.port is coerced to a string and appended to result
So in fact everything convertible to string is accepted here (which would be any) but you will get a string | null from parse().
In real world apps string | number is usually what is needed which is most likely the reason why noone complained yet.
There are several such places in node; e.a. you can set everything convertible to string as a http header.
* [node] url.format can take a string * [node] http.request / https.request can take a string * [node] reorder Url properties to match ordering in docs * [node] DRY out the Url and UrlObject types * [node] backport split Url / UrlObject types to v0 and v4 * remove 'any' union for UrlObject.query type
8 participants
npm run lint package-name(ortscif notslint.jsonis present).If changing an existing definition:
Increase the version number in the header if appropriate.If you are making substantial changes, consider adding atslint.jsoncontaining{ "extends": "dtslint/dt.json" }.