Skip to content

Remove unnecessary dependencies#8843

Merged
daviwil merged 1 commit intomasterfrom
jiach/removeDependencyOnTypesNode
May 11, 2020
Merged

Remove unnecessary dependencies#8843
daviwil merged 1 commit intomasterfrom
jiach/removeDependencyOnTypesNode

Conversation

@JianpingChen
Copy link
Copy Markdown
Member

Move the following dependencies from "dependencies" to "devDependencies".
"@types/node-fetch": "^2.5.0",
"@types/tunnel": "^0.0.1",

@JianpingChen JianpingChen self-assigned this May 11, 2020
@JianpingChen JianpingChen marked this pull request as draft May 11, 2020 19:48
@JianpingChen JianpingChen assigned daviwil and unassigned JianpingChen May 11, 2020
Copy link
Copy Markdown
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me

@JianpingChen JianpingChen marked this pull request as ready for review May 11, 2020 20:48
@chradek
Copy link
Copy Markdown
Contributor

chradek commented May 11, 2020

@types/tunnel was moved to a devDependency once before in #4541, and moved back to a dependency in #4676, I think because the types from @types/tunnel were referenced in an exported type. Can we test that the built npm artifact still compiles when used in a project with another package (like storage-blob) before merging?

I don't know if @types/node-fetch was intentionally added as a dependency or if that can live as a dev dependency.

@daviwil
Copy link
Copy Markdown
Contributor

daviwil commented May 11, 2020

@chradek Thanks for pulling up that PR, I was also worried that changing these might cause compiler-breaking changes. I ran API Extractor on these changes and it doesn't complain that we aren't exporting a type that is being used in either library. I also tried packing the library and using it in a very basic way, didn't see any compiler issues.

The previous issue we saw with tunnel occurred before we made the switch to node-fetch so it's possible that whatever caused it got removed when we dropped the old Axios support. I think we should be OK here but I'll wait a bit longer before merging this PR so that we can do some extra checks if necessary.

Copy link
Copy Markdown
Contributor

@chradek chradek left a comment

Choose a reason for hiding this comment

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

I downloaded @azure/core-http@1.0.0-preview.2 where we had made @types/tunnel a dependency again. @daviwil is right, the axios http client needed the types from tunnel. Now that we don't use axios I'm not concerned about tunnel given David's testing above.

@daviwil
Copy link
Copy Markdown
Contributor

daviwil commented May 11, 2020

Thanks for the extra verification @chradek! I'll merge this now. Thanks for sending the PR @JianpingChen!

@daviwil daviwil merged commit e80ed82 into master May 11, 2020
@daviwil daviwil deleted the jiach/removeDependencyOnTypesNode branch May 11, 2020 22:08
jeremymeng added a commit to jeremymeng/azure-sdk-for-js that referenced this pull request Jun 3, 2020
After that change, @types/node are no longer installed in-directly,
which caused TypeScript compilation for consumers of @azure/core-http
if they don't have @types/node as their dependencies.
jeremymeng added a commit that referenced this pull request Jun 4, 2020
After that change, @types/node are no longer installed in-directly,
which caused TypeScript compilation for consumers of @azure/core-http
if they don't have @types/node as their dependencies.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants