dd-trace-js icon indicating copy to clipboard operation
dd-trace-js copied to clipboard

move @types/node to devDependencies

Open acommodari opened this issue 3 years ago • 4 comments

What does this PR do?

Move the @types/node package from dependencies to devDependencies.

Motivation

Reduce dd-trace-js package size by removing unnecessary prod dependency.

Plugin Checklist

Additional Notes

acommodari avatar Jan 19 '22 01:01 acommodari

I thought that for TypeScript to work properly you need to either depend on TypeScript projects, or to include their definitions in your dependencies. Since we use Node built-in modules, doesn't that mean we need to include the types in our dependencies? I'm definitely no TypeScript expert, so please let me know if my understanding is incorrect.

rochdev avatar Jan 19 '22 02:01 rochdev

Hmmm maybe I jumped the gun with this PR... Honestly whether to include @types in dependencies or devDependencies as a library is not a straightforward decision... I myself don't have much experience with creating Typescript compatible libraries only consuming them.

From what I understand since you are using types from @types/node in your own type declarations those typings would be missing if the package was moved to devDependencies and the consumers of this library did not have @types/node installed locally? Please correct me if I'm wrong.

In the case of @types/node specifically though I think it is a special case. Can we assume that anyone writing Typescript with the node runtime will by default always include @types/node as a prod or dev dependency?

The only edge cases I can think of would be if they do have the package installed locally but they have a version < v12. That and if they were writing a plain Javascript project but were taking advantage of their IDE like VSCode for example to give them better typing information as a kind of bonus.

I don't think there is a right or wrong answer here. There is no perfect solution dependencies, devDependencies, or even peerDependencies.

In my case, I am using dd-trace-js in a private repo for work and we build our project and exclude devDependencies before deploying it to aws lambda. Since what we are deploying is js we don't need any types but I noticed dd-trace-js had typings that wound up in our deployed node_modules folder. Which prompted me to make this PR without really thinking it through as I thought it was an oversight.

I guess lambdas are a special case though because they have hard size limits making users more conscious of the size of their dependencies.

acommodari avatar Jan 19 '22 03:01 acommodari

It's definitely a fair point that when not using TypeScript, the additional dependency is just useless overhead. We're currently looking into optimizing dd-trace for serverless, and we could definitely remove the dependency completely but we'd first need to add an integration test to ensure that your theory is correct and the types from the main project would also work for libraries.

rochdev avatar Jan 19 '22 16:01 rochdev

It looks like this broke one of the benchmarks. Once that's fixed then we can merge this PR 👍 .

rochdev avatar Jan 25 '22 01:01 rochdev

It looks like this PR got a bit stuck for the past year. Is this something you'd still be interested in landing?

tlhunter avatar Dec 20 '23 00:12 tlhunter

This appears to have already been done so closing this.

Qard avatar Jan 02 '24 10:01 Qard