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

@types/* packages should not be production dependencies #18581

Closed
6 tasks
craxal opened this issue Nov 8, 2021 · 6 comments
Closed
6 tasks

@types/* packages should not be production dependencies #18581

craxal opened this issue Nov 8, 2021 · 6 comments
Assignees
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that

Comments

@craxal
Copy link
Member

craxal commented Nov 8, 2021

  • Package Name: @azure/core-http
  • Package Version: 2.2.2
  • Operating system: Windows, Mac, Linux
  • nodejs
    • version: N/A
  • browser
    • name/version: N/A
  • typescript
    • version: N/A
  • Is the bug related to documentation in

Describe the bug
No @types packages should be production dependencies.

To Reproduce
Examine the package.json file for core-http. There are two @types packages in the dependencies section.

Expected behavior
Any @types packages should be dev dependencies.

Screenshots
N/A

Additional context
Applications, such as Storage Explorer, should only ship with files needed to run the app. TypeScript artifacts and types do not need to ship, because they don't contribute any executable code to an app. However, if it's dependencies declare @types packages as production dependencies, we have to write custom build scripts to clean out the unnecessary @types packages.

@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Nov 8, 2021
@ramya-rao-a ramya-rao-a added Azure.Core Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Nov 9, 2021
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Nov 9, 2021
@jeremymeng
Copy link
Member

One reason we had the @types package in the dependencies is that we want provide TypeScript developers with good out-of-box experience: they install our package, import something and the code compiles fine. They don't have to install additional @types packages (especially @types/node which is like a platform). Removing them would be a breaking change.

However, there are benefits of not putting them in the runtime dependencies. We had discussions on making @types/node a peer dependency but didn't have a conclusion.

@bterlson @xirzec @deyaaeldeen @witemple-msft for additional thoughts.

@bterlson
Copy link
Member

bterlson commented Nov 9, 2021

Types which contribute to our public surface area must be normal dependencies, we can't have additional steps for TS developers to get started, and this is common practice for this reason. If we have @types for dependencies which don't contribute to public surface area, they should be removed. I'm not sure if that's the case for @types/node-fetch or @types/tunnel off hand but we can investigate for that specific case..

@deyaaeldeen
Copy link
Member

Types which contribute to our public surface area must be normal dependencies

Just a thought: If we want to be truly independent/isomorphic, I think our public surface will have to be self-contained by shimming what appears in it from the runtime/3rd party packages.

@jeremymeng
Copy link
Member

We thought the focus should be on core v2, so not worth to investigate in core-http.

@witemple-msft
Copy link
Member

@bterlson I agree with @deyaaeldeen when it comes to types for features that are provided by a runtime environment. If the type is part of the public surface area and comes from a corresponding dependency then the types should be a dependency as you're saying, but when it's for a runtime like @types/node or from dom.d.ts, then we should shim it. I don't know if it's worth repairing core-http to this standard versus building corev2 for it.

We use Buffer in a lot of public surface methods, so we should probably be shimming it the same way we shim certain DOM types. I'm pretty certain most of our packages currently require a sibling dependency on @types/node somewhere in the graph due to these signatures.

@xirzec
Copy link
Member

xirzec commented Mar 31, 2023

Unfortunately, we were never able to remove this dependency without breaking existing scenarios, but the good news is that core-http will soon be deprecated as our last packages move off of using it.

@xirzec xirzec closed this as completed Mar 31, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jun 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that
Projects
None yet
Development

No branches or pull requests

7 participants