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: export types independent of @types/request #44

Merged
merged 3 commits into from
Jul 14, 2019
Merged

Conversation

JustinBeckwith
Copy link
Contributor

@JustinBeckwith JustinBeckwith commented Jun 21, 2019

I'm not 100% if this is the right answer, but here we go. This change drops the dependency on @types/request, and exports its own set of types to be used by consuming clients.

The primary reason for doing this comes from here:
googleapis/nodejs-common#392

  • Since this module didn't export it's own types, consumers need to install @types/request
  • Some of the request interfaces are directly exposed in public methods. As a result, @types/request needed to be installed as a dependency instead of a devDependency
  • Folks run npm ls, and see that a @types package leaked into their production code

The secondary reason is being explicit about which fields are supported and not supported. While the goal is compatibility with request, we have seen cases (like the forever flag) where a feature was missing, but the types provided by @types/request tell the user everything is fine. It would be nice to use the type system as a way to signal what is and isn't specifically supported.

I can't really think of an alternative approach that doesn't lead to leaking these packages into production builds. Would love to hear others thoughts!

@ofrobots @benco @fhinkel @alexander-fenster @stephenplusplus @callmehiphop

@JustinBeckwith JustinBeckwith added do not merge Indicates a pull request not ready for merge, due to either quality or timing. needs work This is a pull request that needs a little love. labels Jun 21, 2019
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 8, 2019
@JustinBeckwith JustinBeckwith removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. needs work This is a pull request that needs a little love. labels Jul 8, 2019
@JustinBeckwith JustinBeckwith requested review from fhinkel and bcoe July 8, 2019 03:13
Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

I like this change:

  1. we're no longer using this library as a drop in replacement for Request in our libraries, so I think inheriting its types is less necessary.
  2. it's a pain to have to keep in step with request, given .1.

@JustinBeckwith JustinBeckwith merged commit fbe2b77 into master Jul 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants