search

jaydenseric / graphql-upload

Middleware and an Upload scalar to add support for GraphQL multipart requests (file uploads via queries and mutations) to various Node.js GraphQL servers.
https://npm.im/graphql-upload
MIT License
1.43k stars 131 forks source link

feat: produce TS declaration files #317

Closed SimenB closed 2 years ago

SimenB commented 2 years ago

This allows consumers not to set allowJs and maxNodeModuleJsDepth in order to consume the types

jaydenseric commented 2 years ago

Thanks for your interest in contributing to the project!

For a long time (over a year) I investigated the best possible way to add type safety to all my packages that works universally in Node.js TypeScript projects as well as Deno projects, without a build step. I consciously decided generating type definition files was not part of the solution, for reasons (briefly) explained here:

https://github.com/jaydenseric/graphql-upload/issues/282#issuecomment-1143406053

JavaScript modules containing TypeScript JSDoc comment based types are incredibly portable. Deno users can import them from almost any CDN and enjoy the types without doing anything special, and it's really easy for Node.js TypeScript users to tweak their TypeScript config to get a similar experience.

SimenB commented 2 years ago

The "build step" is already in the repo and part of your prepublishOnly script, you just don't keep any artifacts from it. So this doesn't add any new steps or require a single change to your flow.

Requiring users to change away from the default tsconfig just to consume your library seems an unnecessary complication. Especially when you don't even wanna document it. It working out of the box for Deno is great, but that shouldn't be used as a reason not to work out of the box with Node and TypeScript, which has orders of magnitudes more users.

jaydenseric commented 2 years ago

The situation is more nuanced than you may realise. For example, what happens when a Deno user imports a module from a package that has both JSDoc types and type definitions (like proposed by this PR) via a CDN like esm.sh, that detects there are type definition files, does a bunch of work to resolve and build Deno friendly type definition files for that version of the package if it is not yet built, and then adds them in the response in a X-TypeScript-Types header? I'm pretty sure a loading/caching waterfall event will happen where Deno will then go out and fetch the definition files, and any other definitions that definition imports, and so forth, and cache them all to disk, redundantly when it had the JSDoc types available in the original module's response all along it could have used.

There are more things to consider I haven't even mentioned yet. For example, when using JSDoc based types command clicking on them in your project code opens the actual module you are using them from and takes you to the right place, instead of opening some definition file that is missing all of the context the actual implementation code gives you. If we added definition files on top of the JSDoc comments, we would lose that DX.

The way my package types script currently works is not a build step; it doesn't emit any files and is not necessary to run while developing the package. It's a type check script for CI, in the same way that Prettier or ESLint scripts are not build scripts but check scripts. The proposed changes in this PR introduce a build step; you are generating and not manually authoring those type definition files that get published.

iainjreid commented 2 years ago

For a long time (over a year) I investigated the best possible way to add type safety to all my packages

Silver bullets are rare, and in this instance I feel the sheer number of TS related comments in many of the projects issues regarding the types (not) exported by this package, tells the true story.

JSDoc is a documentation tool, a great one, but it's only that. It can't handle complex generic types, higher kinded types, conditional types, type narrowing, I could go on; ergo, choosing it as the single option to document all of your packages seems a little ambitious.

I've been down this road before with many small packages, and have almost always ended up following the popular maintainers approach of writing JS and "hand crafting" TSD files on the side. They sit right next to the files that they describe (avoiding the issue of context that you rightly mentioned), and do a much better job describing the type level information than JSDoc could ever do on its own.

This approach requires no tooling, no extra build steps, is easy to maintain, and future proofs your project in the event that you require any features that JSDoc simply cannot handle.

I'm seeing this package bundled in with many Apollo codebases these days, and I really think that they would all benefit from either this PR, or a straight up rewrite of the types as raw TSD files.