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

Always set options.url when possible #1753

Closed
2 tasks done
mnmkng opened this issue Jun 16, 2021 · 5 comments
Closed
2 tasks done

Always set options.url when possible #1753

mnmkng opened this issue Jun 16, 2021 · 5 comments
Labels
enhancement This change will extend Got features

Comments

@mnmkng
Copy link
Contributor

mnmkng commented Jun 16, 2021

Describe the bug

  • Node.js version: 14, 15, 16
  • OS & version: MacOS 11.2.3

Actual behavior

This applies to the version of got that's in the main branch. We're trying to migrate to it because we need http2-wrapper@2 for our use case.

When you pass an invalid option to got, the options do not get merged correctly or something else happens and the handlers are invoked without actually having access to the provided options. It's most likely caused by the newly added strict validation of options.

If you run the code example below, you'll see that console.log(options.foo) prints undefined and the No URL error will be thrown.

Expected behavior

I would expect an Unexpected option: foo error to be thrown. Not the handlers to be called with missing options. If I remove the handlers, the code behaves as expected. It also works fine in Got 11, albeit without the Unexpected option error.

Code to reproduce

import got from 'got';

const got2 = got.extend({
    handlers: [
        (options, next) => {
            console.log(options.foo)
            if (!options.url) throw Error('No URL');
            return next(options);
        }
    ]
})
const response = await got2({
    url: 'https://example.com',
    foo: 'bar',
})
console.dir(response);

Checklist

  • I have read the documentation.
  • I have tried my code with the latest version of Node.js and Got.
@mnmkng
Copy link
Contributor Author

mnmkng commented Jun 16, 2021

It also seems that the Unexpected option: foo error comes "later" as an unhandled rejection:

[UnhandledPromiseRejection: This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch().
The promise rejected with the reason "RequestError: Unexpected option: foo".] {
  code: 'ERR_UNHANDLED_REJECTION'
}

@szmarczak szmarczak added the bug Something does not work as it should label Jun 16, 2021
@szmarczak szmarczak added bug Something does not work as it should enhancement This change will extend Got features and removed bug Something does not work as it should labels Jul 1, 2021
@szmarczak
Copy link
Collaborator

I wasn't able to reproduce the unhandled promise rejection, but I managed to set options.url if possible.

@szmarczak
Copy link
Collaborator

Not the handlers to be called with missing options.

But then Got would not know where to throw the error at. Is it a Promise? Is it a Stream?

@szmarczak
Copy link
Collaborator

If you can reproduce the unhandled promise rejection, please open another issue and I'll fix it.

@szmarczak szmarczak changed the title Invalid options break handler execution in Got 12 (main branch) Always set options.url when possible Jul 1, 2021
@mnmkng
Copy link
Contributor Author

mnmkng commented Jul 25, 2021

I tried to reproduce the unhandled rejection again and was not able to. I guess your latest changes must have fixed it. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This change will extend Got features
Projects
None yet
Development

No branches or pull requests

2 participants