Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive TypeScript type definitions to the content-disposition package, transitioning type management from DefinitelyTyped to the package repository itself.
Key Changes:
- Adds TypeScript type definitions in
index.d.tswith interfaces forContentDispositionandOptions - Configures package for TypeScript with
tsconfig.jsonextending@tsconfig/node18 - Adds type testing infrastructure using
expect-typeand@arethetypeswrong/cliwith CI integration
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| index.d.ts | Defines TypeScript types for the contentDisposition function, parse method, and related interfaces |
| package.json | Adds type-related fields (type, main, types), devDependencies for TypeScript tooling, and test-types script |
| tsconfig.json | Configures TypeScript compiler to extend Node 18 base configuration |
| test/types.ts | Provides type checking tests using expect-type to verify API contracts |
| .github/workflows/ci.yml | Adds test-types job to CI pipeline for automated type checking |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Would you be interested in making a PR to use TypeScript and generate the types instead? I'm more comfortable doing that personally. Also just a heads up but I think this will be a major change based on discussions about the policy of types IIRC, so I'd love to bundle it with some meaningful rewriting for performance and removing regexes in the next major. |
|
Hey @blakeembrey, I’m not sure we actually want to rewrite every module to TypeScript. As far as I remember, and since I’ve been in the project, there has been opposition to rewriting every package to TypeScript. The solution for adding types was to create a working group to add and maintain the types. |
I believe I got it to the point where it was the captains discretion, and I'm opposed to maintaining independent types and in favor of native TypeScript. Happy to rehash that discussion though. |
|
I'm fine with rewriting to TypeScript and support this decision. |
If that’s the case, I’m happy to continue and to help you migrate your packages to TypeScript. And honestly, I support it being at the captains’ discretion. |
This PR adds type definitions to
content-dispotition. It includes the following changes:@types/content-dispositioninto the repository (https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/content-disposition)type,mainandtypesfield topackage.jsontsconfig.jsonwhich extends@tsconfig/node18from https://github.com/tsconfig/bases@arethetypeswrong/cliandexpect-typeRef: expressjs/typescript-wg#1
I plan to further improve the types once they are merged.
cc @bjohansebas @jonchurch @clicktodev