refactor: Make graphql-upload great again 🇺🇸

[bnussman]

Why ❓

• This package has gotten completely messed up for many people and we need to stop pretending that it isn't this package's fault
  • export graphqlUploadExpress #374
  • Hard to find FileUpload interface. #369
  • Error [ERR_REQUIRE_ESM]: require() of ES Module not supported #368
  • CommonJS support #360
  • Error [ERR_REQUIRE_ESM]: require() of ES not supported #356
  • const GraphQLUpload = require("graphql-upload/GraphQLUpload.mjs") error but i dont use require #346
  • Please support cjs beside esm #329
  • I can keep going, but I won't

Description ✍🏼

• Re-write from Javascript to Typesript
• Uses Vitest to run unit tests
• Uses tsup to build esm and cjs
• Exports the types and functions in a way where it is easy to import them
• Uses pnpm as the package manager

Notes and Disclaimers 🗒️ ⚠️

• The diff makes it look like I added a lot of junk, but it's just because I committed the lock file. This is recommended practice and I don't know why it wasn't done before.
• I removed eslint and prittier to de-bloat this repo but if @jaydenseric admits this PR is the right direction to move in, we can add these back

Next steps ✅

1. Get @jaydenseric to admit this is much better than what this repo currently does
2. If we accomplish step 1, we can make this PR more "production ready"

[jaydenseric]

Re-write from Javascript to Typesript

This is something I deliberately don't do for published packages I maintain, based on years of experience using and maintaining packages. It can't be overstated how much better it is directly authoring the same code users consume when they install the package.

• The code you write and review is the final code; there are no surprises about what your build tooling produces in the build artifacts.
• Without a build step there is less dev dependencies, config, scripts, etc.
• You can have a simpler directory structure for the published modules because you don't need to nest code in src and dist directories, which means consumers have shorter import paths to write. This is more ergonomic, but also translates to less bytes over the wire when using ESM in Deno or browsers and the modules are downloaded over HTTP and cached.
• The published modules are formatted and commented nicely, without being messed up by build tooling which can reformat the source code and eliminate or move comments. This makes for a better debugging experience when investigating stack traces in your project.
• There are no source maps, which means the install size of the package is significantly smaller. This also makes it incredibly intuitive to debug the package and contribute changes because the source and published modules are the same files.
• In the context of TypeScript, if you publish typings separately in type definition files, there are significant downsides.
  • When you CMD-click in your project code to go to types originating in the published package, it takes you to a type definition file instead of the actual implementation code you are interested in seeing.
  • Separate type definition files need to be declared in special ways in the implementation module so they are discoverable by Deno, and Deno has to then cache the extra file in a waterfall loading step over HTTP when caching the modules. When the types are located in the implementation module via TypeScript flavor JSDoc comments, none of those downsides exist.

I shouldn't have to justify inherently good engineering ideas by pointing out other famous people also do it that way, but if it sways your bias consider that Webpack, Svelte, and an increasing number of other OSS projects have been moving to JavaScript source with TypeScript type safety via JSDoc comments:

https://twitter.com/Rich_Harris/status/1661051005985865728

Uses Vitest to run unit tests

This is a more bloated approach than my old test setup, which was working fine. Over time I plan to move my open source projects to the built in Node.js test runner, which is a zero dependency solution.

Uses tsup to build esm and cjs

It's an anti-pattern to publish dual CJS and ESM modules in the same package. Learn more here:

• https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c?permalink_comment_id=3850849#gistcomment-3850849
• https://jaydenseric.com/blog/optimal-javascript-module-design

The dual package hazard is a very real problem that I have encountered in real world situations.

Future generation will find it mind-boggling that anyone thought it was a good idea to publish and try to consume our software in two formats at once; one web standard and one not.

Exports the types and functions in a way where it is easy to import them

If you are referring to publishing definition files, I already explained why that is undesirable. If you are referring to publishing a main index module, then that is absolutely unnecessary and an optimal module design anti-pattern.

Uses pnpm as the package manager

No thanks, it makes this project a bit more complex and we should be testing with npm as that is the what the majority of the package consumers are using.

The diff makes it look like I added a lot of junk, but it's just because I committed the lock file. This is recommended practice and I don't know why it wasn't done before.

Package lockfiles are a good idea for app projects, but they are an anti-pattern in repos for published packages. npm doesn't publish the lockfile; users installing your package get whatever the latest versions are of the dependencies that are specified in the package dependencies field. If we have a lockfile in this repo, all that will do is prevent our local development and CI from noticing changes affecting our end users. This is not a good thing. Lockfiles also make for more complicated diffs that are hard to review when contributions affect dependencies.

I'm not the only open source author that sees things this way:

sindresorhus/ama#479 (comment)

I removed eslint and prittier to de-bloat this repo

That's crazy you would think removing linting and formatting is a good idea, even temporarily.

I could continue to review the other changes in this PR, like how the package exports field was removed, etc. but I don't have the time right now to explain more things. Bundling the fs-capacitor dependency because it's also ESM is a terrible idea for many reasons. Also, your comment suggesting the fs-capacitor author is "lazy" is toxic:

https://github.com/jaydenseric/graphql-upload/pull/376/files#r1271358606

If you configure TypeScript correctly in your app project that consumes graphql upload, you can use it with full type-safety no problems at all; I do so at work. It's not my job to care about all the legacy codebases that exist that don't want to update their tooling or config, yet non-sensibly want their dependencies to continue to work for them forever through every major update. I have clearly documented all the major changes over time in the changelog, it's up to users to migrate.

You seem intent on publishing a graphql-upload fork; make sure you do the authorship and licensing correctly as per the the terms of the MIT license this project has:

https://github.com/jaydenseric/graphql-upload/blob/v16.0.2/license.md

[jaydenseric]

This comment accusing the fs-capacitor author of being lazy is toxic. He's put a lot of work into that package for free; his decision to publish pure ESM was not a lazy one.

[bnussman]

My fault. Respect to that guy. I just was moving fast and wanted to get things working.

[jaydenseric]

To put out ESM compatible with Node.js you must use module set to nodenext (which also sets moduleResolution to nodenext). node should not be used anymore as it doesn't reflect how current versions of Node.js work; the TS team didn't update it's behavior because they didn't want to impact legacy codebases using it.

[jaydenseric]

By committing all this commented out crap it's clear this PR is a form of protest and isn't intended to be a quality contribution.

[bnussman]

I wan't going to do quality work until I knew where you stood on this. This was opened as a draft for a reason.

[jaydenseric]

You have to use JSDoc comments (starting with /**) for them to show up in intellisense.

[bnussman]

Was moving fast just to get things working. Chat Jippity did this bit of code

[jaydenseric]

You likely wouldn't have messed up the line ending here had you not deleted the .editorconfig file.

[bnussman]

Just trying to learn here... What do you mean the line ending is messed up? If the .gitignore file works, I could care less.

[jaydenseric]

Closing, as per #376 (review) .

[bnussman]

Thank you for that very interesting perspective. Also, thank you for your time and development of this package. I respect your work and I do not intend to infringe on anything. Please let me know if these is any action I need to take to do so.

Also, thank you for the critical code review. While I disagree with some things, I learned your viewpoint and reason for doing things the way you do them.