Skip to content
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

fix: throws on unsupported option path #1282

Closed
wants to merge 2 commits into from
Closed

Conversation

is2ei
Copy link
Contributor

@is2ei is2ei commented Mar 16, 2022

Refs: #1011

undici.request accepts path option but the type definition omits path.
POC: https://github.com/is2ei/POC-nodejs-undici-1011
code:

undici/index.js

Lines 63 to 75 in d7eac3e

if (opts && opts.path != null) {
if (typeof opts.path !== 'string') {
throw new InvalidArgumentError('invalid opts.path')
}
url = new URL(opts.path, util.parseOrigin(url))
} else {
if (!opts) {
opts = typeof url === 'object' ? url : {}
}
url = util.parseURL(url)
}

README.md Outdated Show resolved Hide resolved
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

path is already part of these through extending parent structure. What is the problem you are trying to solve?

@is2ei
Copy link
Contributor Author

is2ei commented Mar 16, 2022

@ronag
Thanks for your comment. I basically try to solve 2 problems related to #1011.

  1. wrong type definition
  2. document for undici.request lacks some information.

When using TypeScript, you can not pass the path option to undici.request because the type definition omits the path option. But when using JavaScript, you can pass the path option to undici.request and it works. So I fixed the type definition.

undici/index.js

Lines 63 to 68 in d7eac3e

if (opts && opts.path != null) {
if (typeof opts.path !== 'string') {
throw new InvalidArgumentError('invalid opts.path')
}
url = new URL(opts.path, util.parseOrigin(url))

Also, I found method option is documented even though DispatchOptions already has it. I guess the difference is method is mandatory in DispatchOptions but optional in undici.request option. path option has the same situation so I documented it.

undici/types/api.d.ts

Lines 13 to 17 in d7eac3e

/** Performs an HTTP request. */
declare function request(
url: string | URL | UrlObject,
options?: { dispatcher?: Dispatcher } & Omit<Dispatcher.RequestOptions, 'origin' | 'path' | 'method'> & Partial<Pick<Dispatcher.RequestOptions, 'method'>>,
): Promise<Dispatcher.ResponseData>;

@ronag
Copy link
Member

ronag commented Mar 16, 2022

When using TypeScript, you can not pass the path option to undici.request because the type definition omits the path option. But when using JavaScript, you can pass the path option to undici.request and it works. So I fixed the type definition.

I think the typescript version is correct and reflects intended/recommended usage.

Whether or not the javascript version should allow it is up for discussion. Maybe we should throw?

@is2ei
Copy link
Contributor Author

is2ei commented Mar 16, 2022

@ronag

Maybe we should throw?

Ok. I'll try.

@is2ei is2ei changed the title fix: correct type for path option fix: throws on unsupported option path Mar 16, 2022
@codecov-commenter
Copy link

codecov-commenter commented Mar 16, 2022

Codecov Report

Merging #1282 (46451d9) into main (d7eac3e) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1282      +/-   ##
==========================================
- Coverage   94.11%   94.11%   -0.01%     
==========================================
  Files          44       44              
  Lines        4098     4096       -2     
==========================================
- Hits         3857     3855       -2     
  Misses        241      241              
Impacted Files Coverage Δ
index.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7eac3e...46451d9. Read the comment docs.

@is2ei
Copy link
Contributor Author

is2ei commented Mar 16, 2022

@ronag
I changed the code. Could you review it?
I basically agree with your opinion, but I'd be worried if it would be a breaking change.

@ronag ronag added the semver-major Features or fixes that will be included in the next semver major release label Mar 22, 2022
Copy link

@hebertcisco hebertcisco left a comment

Choose a reason for hiding this comment

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

I saw that your pull request has conflicts, have you tried to resolve these conflicts?

Copy link

@hebertcisco hebertcisco left a comment

Choose a reason for hiding this comment

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

I saw that your pull request has conflicts, have you tried to resolve these conflicts?

Copy link

@hebertcisco hebertcisco left a comment

Choose a reason for hiding this comment

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

@ronag What do you think of this pull request and the changes made to it? Would it be mergeable and adequate?

@is2ei
Copy link
Contributor Author

is2ei commented May 31, 2023

@hebertcisco
I fixed the conflict, but it looks path option is actually used now.
Do we still need this change?

@mcollina
Copy link
Member

I think this can be closed, thanks!

@mcollina mcollina closed this Jun 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major Features or fixes that will be included in the next semver major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants