feat!: publish first-party type declarations

[IlCallo]

I could not run tests, as they expect a .tedious/test-connection.json to exist in my home folder
I haven't changed anything into the code anyway, so that's not a big problem

Some notes:

• your package-lock.json version is outdated I noticed there's an recent PR to address that
• "types" need the /src/ because you import the project package.json into one of your files and this automatically change TS rootDir. Consider removing the following import to avoid this problem

  tedious/src/connection.ts

  Line 48 in cbe31af

  import { version } from '../package.json';

For everyone coming here to get updated types:

• download my branch
• npm install
• remove

  tedious/src/connection.ts

  Line 48 in cbe31af

  import { version } from '../package.json';

  dependency
• replace "declarationDir": "lib/types" with "outFile": "lib/tedious.d.ts" into tsconfig.build-types.json
• npm run build:types
• copy over the generated lib/tedious.d.ts file into your project
• install @types/es-aggregate-error and node-abort-controller as devDependencies of your project
• commit
• remove when/if this PR gets merged and released

[dhensby]

I don't think there's any need for a separate command/step to output types - they can just be done as part of the standard build by adding the declaration compiler option... Is there an advantage to doing it separately?

I'd suggest there needs to be a CI step to verify the types are indeed being build in this separate step. I note your JSON file has comments, which I don't believe are valid in JSON, so would need removing

[IlCallo]

I don't think there's any need for a separate command/step to output types

You perform build step using Babel, not TSC.
AFAIK Babel isn't capable of type-checking, it only performs transpiling.
That's why you need a separate step, there is no "declaration" compiler option for Babel.

I'd suggest there needs to be a CI step to verify the types are indeed being build in this separate step

Not sure it's that important, unless you want to prevent other people to accidentally disable it
Anyway, I guess you just need to check the lib/types folder exists, everything else in just configuration for TSC
This is an initial PR to at least have first-party types released, since there are multiple issues asking for it, some older than 1 year
Of course it can be enhanced later on

I note your JSON file has comments, which I don't believe are valid in JSON

TypeScript config files are interpreted as JSONC command, which support comments
Unless you plan to programmatically read (possible via json5 library) and update (not currently possible) that file, comments will just be ignored by TypeScript while reading the configuration

[IlCallo]

Not sure why CI is failing, seems to me you're building on a Windows machine, I guess it has some problems with the command format

tedious@0.0.0-dev build:types
tsc --project tsconfig.build-types.json
The filename, directory name, or volume label syntax is incorrect.

Maybe it needs to be tsc --project="tsconfig.build-types.json"? I don't use Windows, if anyone can try it out, I can update the PR

Yet, other commands work, not sure what's the cause then

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.25%. Comparing base (4a6273c) to head (f523e6d).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1490      +/-   ##
==========================================
- Coverage   78.41%   78.25%   -0.17%     
==========================================
  Files          93       93              
  Lines        4860     4860              
  Branches      934      934              
==========================================
- Hits         3811     3803       -8     
- Misses        751      757       +6     
- Partials      298      300       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[WikiRik]

Any chance this PR can be revisited? There's a big PR open over at DefinitelyTyped to update @types/tedious, but implementing it in this repo is better for maintainability.

cc @lohart13

[arthurschreiber]

Okay, so I pushed a few changes to this PR in preparation for merging this.

A few notes. Bundling the type definitions with tedious will require adding @types/node and @types/es-aggregate-error to the dependency group in package.json. We might be able to drop es-aggregate-error and @types/es-aggregate-error because Node.js 18.x includes it, but I've not gotten around to it yet.

But the dependency on @types/node is pretty much required because tedious re-exports a lot of the types (e.g. BulkLoad is an EventEmitter, so we need the type from @types/node).

These dependencies in turn mean that each installation of tedious will pull down @types/node even if tedious is used in a pure node.js project. Are we okay with having this dependency?

[WikiRik]

Not active within this project itself, but I've seen plenty of projects that do have @types/node in either the dependencies or in the installation instructions. Even with pure JS projects I think most do already have it installed. I wouldn't worry too much about it

[dhensby]

Are we okay with having this dependency?

Personally I don't think it's ok to have packages list dev dependencies as prod dependencies just to let types be exported.

Consumers of the library should already have those types pulled in themselves. @types/node is also environment specific, if I'm building an project for node 21, then I want @types/node@21 and not @types/node@18 as the inherited types.

If there are effectively "peer" typings needed, it's for the root project to be pulling those in, really. I wouldn't want @types/* to be deployed in production.

[IlCallo]

I agree on adding TS deps as devDependencies as @dhensby said, not prod ones
This would also reduce type clashes between packages

You could mark as peer deps the ones you need to re-expose and ask the end user to install them, if they use TS or want proper autocomplete

[arthurschreiber]

I've spent some time looking around and there's various posts around this topic. The recommended approach by the TypeScript project itself is to list @types/node (or any other type specific dependency) as a regular dependency (not a peerDependency or devDependency) if a type is re-exported. And that's exactly what we do here.

For examples of this see this discussion DefinitelyTyped/DefinitelyTyped#44777, microsoft/types-publisher#81 (comment), and other popular packages like grpc-js doing exactly that.

If we didn't export any of the types contained within @types/node, listing it as a devDependency would be correct, but we are doing that in various places so if we don't do it, the type definitions will be broken for users of the package.

[dhensby]

Well, it's definitely a matter of opinion - I just know I'd never want to be forcing downstream packages to be pulling in @types/* packages for anything I wrote and I know I do not want to ship @types/* packages to prod (I've raised this issue with the azure-sdk-for-js packages as well (which I believe they worked on getting rid of the prod-deps since).

Typings are an inherently development dependencies and are not needed for production, which is why I would strongly advocate they are never included as production deps.

If we didn't export any of the types contained within @types/node, listing it as a devDependency would be correct, but we are doing that in various places so if we don't do it, the type definitions will be broken for users of the package.

This is only true if consumers of the tedious types didn't include the @types/node types in their dev deps, and not doing that whilst expecting typings seems very odd to me. No doubt you will get people raising issues saying that the types aren't working because they have failed to install the typings themselves, but you'll also get people like me opening issues to complain that typings are making it into their prod builds 😅

[dhensby]

Just to add, for the grpc-js library, we can see they added the @types/node package to prod deps purely to satisfy people who didn't have it installed as a first-class devDependency (see grpc/grpc-node#1538), even though they report it works fine if they install node typings themselves (which is the correct solution IMO).

[arthurschreiber]

Typings are an inherently development dependencies and are not needed for production, which is why I would strongly advocate they are never included as production deps.

That is true, but I think a misunderstanding or difference in interpretation around the difference between devDependencies and dependencies might be the root cause here. To me devDependencies are the dependencies required to do development on a specific package - e.g. all the tools and libraries used when working on tedious. On the other hand, dependencies are what is required to "use" tedious.

So, if we start bundling types inside the tedious package, any type definition that is required when using tedious with typescript needs to be included inside dependencies (for example, @types/node or @types/bl), while type definitions that are only required when working on tedious will go into devDependencies (for example, @types/mocha or @types/chai).

Otherwise, the whole dependency management for type definitions breaks down, and users will have to spend a troubling amount of work of piecing together which types packages and which versions they need to get typing to work. It can also cause all kinds of havoc when different dependencies require different versions of type definitions, because at the top level of the package hierarchy, only a single version of a type library can be defined.

[arthurschreiber]

@IlCallo I know it's been a while since you worked on this, but can you help me understand what the problem around the import from package.json was? I pushed some changes to this branch and I don't really understand what the initial problem was.

[IlCallo]

Honestly I don't remember much, but if you don't have any strange generation issue, then the problem is probably solved already

IIRC the problem with importing the package.json was that you would then end up with broken types and generation as a deeply nested folder
But I finished long ago the project I was working on when I created this PR and I don't have much time to check this again soon

[dhensby]

That is true, but I think a misunderstanding or difference in interpretation around the difference between devDependencies and dependencies might be the root cause here. To me devDependencies are the dependencies required to do development on a specific package - e.g. all the tools and libraries used when working on tedious. On the other hand, dependencies are what is required to "use" tedious.

I think that's right, the devDeps are for development and the deps are for running in procuction. Types are not needed to run in production, so they should be listed as devDeps. I know devDeps don't cascade down, and that's the reason why this typing issue comes up and it's intended behaviour in the dependency management - but it does lead to these kind of edge-cases. The only reason to require the types is so that people who develop with tedious (and need the types) don't get errors if they fail to install the node types themselves - that's entirely a development environment issue (hence my view this is a devDependency).

Otherwise, the whole dependency management for type definitions breaks down, and users will have to spend a troubling amount of work of piecing together which types packages and which versions they need to get typing to work. It can also cause all kinds of havoc when different dependencies require different versions of type definitions, because at the top level of the package hierarchy, only a single version of a type library can be defined.

Unfortunately, I don't quite think this is right,(if you look at the grpc-js resolving PR they note that type resolution only looks at the root typings installed and not specific versions on a per module basis, so if libraries have conflicting types, it's not solved by them declaring them in their own deps. This is another reason why developers should be expected to install their own typings based on the software they consume / the environments they work in.

Perhaps the middle-ground here is to add the typings as a peerDependency, which means devs get a hint that they need it installed, but the tedious package is not bloated with development packages in prod?

[IlCallo]

To solve at least one of the problems of adding the types as deps instead of devDeps, in particular the types mismatch in case another version is used in the project root, you could define it as "@types/node": "*" which should use whatever version of that package is resolved from other packages

It doesn't solve the "bloat" in production, but types are usually removed in a project build step anyway, when the project is transpiled to JS

[arthurschreiber]

@mShan0, @MichaelSun90 and I double checked, and @types/node is pulled in as a sub-dependency by bl and @azure/core-http already. This means that as long as those dependencies pull in @types/node, there is no point in us not declaring @types/node as a dependency of our own.

If that changes in the future, we can re-visit this, but for now, I'd just go ahead and merge this in it's current state.

[github-actions]

🎉 This PR is included in version 18.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀