[core-http] move typings to dev dependency

[dhensby]

Packages impacted by this PR

• core-http

Issues associated with this PR

n/a

Describe the problem that is addressed by this PR

@type/* packages should be devDependencies. This stops typings being distributed in production builds

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

Are there test cases added in this PR? (If not, why?)

It's a dependency issue

Provide a list of related PRs (if any)

Command used to generate this PR:**(Applicable only to SDK release request PRs)

Checklists

• Added impacted package name to the issue description
• Does this PR needs any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here)
• Added a changelog (if necessary)

[ghost]

Thank you for your contribution dhensby! We will review the pull request and get back to you soon.

[dhensby]

I've noticed that types are being included in quite a lot of packages as production dependencies instead of devDependencies.

I'm happy to go through them and update them, but wanted to check this would be accepted before spending that effort.

Perhaps there should be a linting rule in the future to ensure no types make it into production dependencies?

[xirzec]

I remember there being some strange history here.

We brought this back in this PR after some failures came in:
#4676

Then we tried to remove it again:
#8843

Then we reverted that:
#9276

We filed this to fix it:
#9278

But closed that since we weren't investing further into core-http. We still have this external issue open tracking it:

#18581

tl;dr I think we're stuck with a production dependency because we re-use types from these packages

[jeremymeng]

Now that we move core-http to v3, it could be a good time to remove it?

[dhensby]

Interesting and thanks for the context on this.

I can see that this is a complex issue to get right with such intricate interdependencies between all the different packages.

In an ideal world, if a library is relying on another package (and thus its types) it should be exporting those types as first class types rather than relying on consumers to have those typings locally. I've fallen foul of packages not doing this and it can cause some really hard to debug issues.

My rule of thumb has been, if my library relies on a type from another library, I'll export it as part of my package. This prevents consumers having to install types for packages they don't have as first party dependencies and also prevents NPM's duplicate dependencies from causing pesky inconsistent errors.

[xirzec]

@dhensby I think you have the right of it and I believe our new core @azure/core-rest-pipeline is configured correctly:

azure-sdk-for-js/sdk/core/core-rest-pipeline/package.json

Line 87 in a8b1f64

"dependencies": {

[dhensby]

The core-http dependency is coming from applicationinsights dependency. I'm not sure if there are plans to move to core-rest-pipeline for that library

[xirzec]

The core-http dependency is coming from applicationinsights dependency. I'm not sure if there are plans to move to core-rest-pipeline for that library

Thanks for the pointer! I see @jeremymeng already opened an issue there to start this discussion: microsoft/ApplicationInsights-node.js#1033

[dhensby]

Given the history here and that core-http is being somewhat EOL-ed, I will close this issue. I'd like to see the deps removed, but if the effort isn't worth the pay-off I can see it's a fair enough position to just leave it as-is.

[xirzec]

Given the history here and that core-http is being somewhat EOL-ed, I will close this issue. I'd like to see the deps removed, but if the effort isn't worth the pay-off I can see it's a fair enough position to just leave it as-is.

I appreciate your understanding! We'll continue working on deprecating this package so you don't have it pulling in types but please let us know if you see this happening elsewhere.