fix (getAgent): add nullish check for uri

[jsjoeio]

This adds a fix to requestToFetchOptions to throw an exception if uri is missing when calling from teenyRequest.

We should do this because we can't guarantee that the value is always a string
(due to type assertions) or code calling it without a valid string.

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

• Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
• Ensure the tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

Fixes #236 🦕

[JustinBeckwith]

Greetings, and thanks for the PR! Can you share a little more on the motivations here? The getAgent method requires this uri parameter because we use it. What is the scenario where this would be null/undefined?

[jsjoeio]

Hey @JustinBeckwith - thanks for the quick response. Not sure if you saw the original issue which talked about the motivation behind this change, but happy to explain again here.

It looks like there is a type assertion on uri before it gets passed to getAgent here.

As a result, getAgent wrongly assumes uri is a string and calls .startsWith() here causing a runtime error.

I noticed this happening in codecov-node here.

The only real fix needed is agents.ts#L40 but I changed the parameter type annotation from string -> string | undefined due to the type assertion. Happy to make changes/go a different approach if you have a better solution!

[JustinBeckwith]

Got it - thanks for the details! The type assertion you linked to makes the assumption that one of uri or url are going to be set in the request options. The problem is ... I guess someone could make a request with neither!? If someone makes a call to request with no uri or url, what would you expect it to do? I am thinking the most ideal outcome would be we throw, because otherwise I have no idea how it would work. Or am I misreading something?

[jsjoeio]

I guess someone could make a request with neither!?

Exactly! And I think that's what's happening in codecov-node, leading to this error.

I am thinking the most ideal outcome would be we throw, because otherwise I have no idea how it would work

Lol same. That makes sense to me.

In that case, should I check if uri is undefined and throw if it is?

[JustinBeckwith]

Yeah I think throwing with a reasonable error here makes sense. It should be backward compatible, because it was going to throw, just with a weird error anyways. I would throw right after we try to snag the uri here:
https://github.com/googleapis/teeny-request/blob/master/src/index.ts#L120

Thanks for sticking with me here!

[jsjoeio]

It should be backward compatible, because it was going to throw, just with a weird error anyways. I would throw right after we try to snag the uri here

Makes sense. Thanks for the suggestion!

I added two tests as well for this change. Let me know what you think!

Thanks for sticking with me here!

Thanks for being kind, patient and helpful!