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

ClientRequest: Clones request options to prevent mutations #87

Merged
merged 4 commits into from
Mar 21, 2021

Conversation

kettanaito
Copy link
Member

@kettanaito kettanaito force-pushed the 86-got-timeout-exception branch from c0f474d to fc1735f Compare January 13, 2021 09:10
@KnisterPeter
Copy link

@kettanaito Do you need something from me to get this merged?

@kettanaito
Copy link
Member Author

Hey, @KnisterPeter. Unfortunately, even with this change, the issue is not resolved.

This pull request does fix the mutation of ClientRequest options and the got's this.timeout doesn't leak to NRI. But there's still an exception because at some point got tries to perform a request to a :80 port of the host and not the specified port. I've described it here, the issue has been re-opened.

While that's probably an issue on the got side, there's always a chance that NRI can ship an improvement to prevent that behavior from happening.

@KnisterPeter
Copy link

Thanks for explaining. Would be nice to at least see this issue fixed.

@kettanaito
Copy link
Member Author

I don't mind merging this, but the issue won't be solved by this pull request alone. That's why I'd prefer not to merge half-fixes. I hope for your understanding on that.

@KnisterPeter
Copy link

Sure, do as you like. Thanks anyway 😃

@szmarczak
Copy link

I put console.log(url) after https://github.com/mswjs/node-request-interceptor/blob/64c42229cf4d68fe58809e4679bce3592939d9c7/src/interceptors/ClientRequest/ClientRequestOverride.ts#L35 and the URL is http://127.0.0.1:[random_port]/user so it must be something wrong on your side.

@szmarczak
Copy link

@szmarczak
Copy link

req = performOriginalRequest((options as any).url, options) and the test passes

@kettanaito kettanaito force-pushed the 86-got-timeout-exception branch from 64c4222 to 825c720 Compare March 21, 2021 13:59
@kettanaito
Copy link
Member Author

Thank you so much for the investigation, @szmarczak ❤️ You're right, we didn't pass the correct url to the original http request.

I've set it to propagate the url parsed from the ClientRequest parameters:

req = performOriginalRequest(url.toString(), options)

That way we don't have to typecast the url to any. Outstanding investigation powers.

@kettanaito kettanaito requested a review from marcosvega91 March 21, 2021 14:04
Copy link
Member

@msutkowski msutkowski left a comment

Choose a reason for hiding this comment

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

Looks good!

@szmarczak
Copy link

I'm glad I could help ♥️

@kettanaito kettanaito merged commit 564dd71 into master Mar 21, 2021
@kettanaito kettanaito deleted the 86-got-timeout-exception branch March 21, 2021 14:59
@kettanaito
Copy link
Member Author

You are incredibly helpful, @szmarczak. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants