-
Notifications
You must be signed in to change notification settings - Fork 30.5k
Node: remove redundant widen of query type to any + tidy up #20720
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
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? |
| export interface Url extends UrlObject { | ||
| export interface OutputUrlObject extends InputUrlObject { | ||
| port?: string; | ||
| query?: any; |
There was a problem hiding this comment.
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)
types/node/index.d.ts
Outdated
| 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; |
There was a problem hiding this comment.
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)
|
@OliverJAsh Please fix the failures indicated in the Travis CI log. |
|
Travis build failed due to timeout, but there were no errors, so this should be good. |
Flarna
left a comment
There was a problem hiding this 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.
Thanks, will revert! |
This reverts commit 0e652b0.
|
@Flarna Reverted now |
types/node/index.d.ts
Outdated
| slashes?: boolean; | ||
| } | ||
|
|
||
| export interface Url extends UrlObject { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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;
}There was a problem hiding this comment.
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 }
}There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
@OliverJAsh Please fix the failures indicated in the Travis CI log. |
|
@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.
|
|
Ping @RyanCavanaugh @Flarna. Build failed due to timeout. Please can we merge? |
Support for index signature properties via dot notation was only added in TS 2.2 https://github.com/Microsoft/TypeScript/wiki/What%27s-new-in-TypeScript#dotted-property-for-types-with-string-index-signatures
|
Approved by a listed owner. PR ready to merge pending express review by a maintainer. |
|
This change is very heavily breaking for many consumers. There's a very good chance anyone (like us!) who did things with |
|
is the submitted type for |
|
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.
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 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 If the concern is just around url.parse, I'd suggest a function overload for that, leaving the original interface alone. |
|
I will investigate adding an overload to fix that use case but it could be
a while before I get to it.
…On Wed, 13 Dec 2017, 16:03 Connor Peet, ***@***.***> wrote:
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 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 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.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#20720 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA4QCRXfllJXAeZc4NPMk-_ewZQOlnsgks5s__VDgaJpZM4P_HOc>
.
|
|
to be continued at #22171 |
Node: remove redundant widen of query type to any + tidy up
Please fill in this template.
npm run lint package-name(ortscif notslint.jsonis present).Select one of these and delete the others:
If changing an existing definition:
tslint.jsoncontaining{ "extends": "dtslint/dt.json" }.