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.42k stars 131 forks source link

support default tsconfig compilerOptions.module #351

Closed revero-doug closed 1 year ago

revero-doug commented 1 year ago

an optimal module configuration in a public library promotes interoperability over all other concerns. please consider updating the library to maximize compatibility with typical tsconfig settings.

jaydenseric commented 1 year ago

Firstly, there is no "default" TypeScript config.

It seems you are asking this library to configure compilerOptions.module to the legacy "node" option (instead of using the "nodenext" option that superseded it) which isn't capable of resolving standard Node.js packages using stable Node.js LTS features like the package exports and type fields, or ESM and CJS interop via the .mjs file extension, etc. This is not a viable suggestion, because this package uses a lot of those things, and needs to resolve and process other modern Node.js packages correctly too.

a public library promotes interoperability over all other concerns

That is why all my open source projects have moved to standard ESM module format, and the most current TypeScript config for compatibility with the current Node.js API. In time, we won't need to care about legacy CJS interoperability concerns and it will all just be a bad memory like legacy IE is. To get there, library authors like me have to make the first moves or it will never happen. Fortunately, lots of other authors are making the same moves and have migrated their packages over to pure ESM.

revero-doug commented 1 year ago

This is not a viable suggestion, because this package uses a lot of those things, and needs to resolve and process other modern Node.js packages correctly too.

this is circular reasoning, as it reduces to "supporting legacy formats is not viable because this library doesn't support legacy formats". this library didn't have a problem with legacy formats for 13 major versions, apparently.

That is why all my open source projects have moved to standard ESM module format, and the most current TypeScript config for compatibility with the current Node.js API [...] in time we won't need to care about legacy CJS interoperability [...] to get there, library authors like me have to make the first moves

ok this is at least an actual argument. breaking clients because you think it will hasten adoption of latest standards is definitely a bug, not a feature when your library is popular enough to be linked from Apollo docs. so far you've offered no reason your particular library needs to be incompatible, or at least that it would incur significantly higher maintenance costs (really not sure what new feature work is needed on something like this) by not breaking clients. you're obviously free to do what you wish with your projects, and as a customer (albeit nonpaying) of your product (albeit free), I hope you'll take this feedback in stride and maybe reconsider when it's appropriate to push a highly opinionated stance that, LTS or not, is bleeding edge.

jaydenseric commented 1 year ago

You seem to have a very narrow definition of "interoperability" that means compatibility with your specific legacy TypeScript Node.js CJS setup. CJS is completely not operable in browsers; ESM is. There are other ESM only runtimes as well. My packages are all moving to be standard JavaScript modules that work in any standard JavaScript runtime. I have real websites up and running that the full back and front end is running on pure ESM without even a build step, using https://ruck.tech (for a list, see https://github.com/jaydenseric/ruck#examples). I couldn't have built the Ruck framework or buildless websites if my packages were not pure ESM; so yes there is real practical value in doing it not just to "push a highly opinionated stance". How can using standard JavaScript be "opinionated" lol.

revero-doug commented 1 year ago

This is not just pedantic but wrong ^.

Firstly, there is no "default" TypeScript config.

There is, every optional compiler option has a default value. A minimally specified config is the default.

You seem to have a very narrow definition of "interoperability" that means compatibility with your specific legacy TypeScript Node.js CJS setup.

My definition for "interoperability" of a library is source compatibility. You admittedly see forcing your consumers to refactor to the new "standard" (as if JS isn't wildly fragmented with de facto standards and de jure standards seldom matching) as a feature, not a bug, when you are by definition introducing source incompatibility.

I have real websites up and running that the full back and front end is running on pure ESM without even a build step, using https://ruck.tech/

... and here is your narrow definition of "interoperability", got it.

I hope you'll take this feedback in stride and maybe reconsider when it's appropriate to push a highly opinionated stance that, LTS or not, is bleeding edge.

You vacillate between saying "it's important to break 'legacy' standards so we can drop the baggage of legacy support" and denying there's breakage at all. It's naive that you think cjs is the sort of "legacy" that ie6 is. It's only legacy in name.

How can using standard JavaScript be "opinionated" lol.

Your closed issues are littered with people giving you negative feedback about this particular opinionated stance you've taken, and the emojis on those comments indicate we're in good company. (an example)

jaydenseric commented 1 year ago

To better explain what I meant by there being no default TypeScript config, what I meant is there is not just ONE TypeScript default config. Deno has it's own default config, tsc in Node.js has it's own default config, etc. etc. Configuring module resolution to nodenext actually aligns with the Deno behavior, because you need to enforce file extensions in import paths. I've deeply considered (and tested) compatibility across runtimes and type checkers when choosing my open source TypeScript config.

At some point, I will swap out the Node-isms in this package, and it will work for Deno too, as well as any other runtimes that provide standard web APIs. It's a long term vision that could suddenly become a short term one if I have to build a Deno GraphQL API with file uploads.

revero-doug commented 1 year ago

this library is basically unusable in even a relatively greenfield typescript project because the community and ecosystem do not agree with you on standards. Even the narrower graphql ecosystem. using graphql codegen and apollo client, operation data comes back as any because this breaks typescript conventions (read: de facto community standards). I guess I'm grateful I didn't adopt this library before it was needlessly broken in the name of purity.

jaydenseric commented 1 year ago

I'm using this library at work right now in an enterprise Node.js TypeScript project just fine. If you are having TypeScript errors with Apollo projects, raise issues there, not here.

My package does exactly what it says in the readme; nothing you are complaining about is a bug. Everything is functioning as meticulously designed. You just disagree with my vision. Well, it's not your project and I'm allowed to have a vision. You're not paying for it, and it seems you're not even a user of it yet you are trying to bully us to completely change the direction of the project. You've expressed your opinions (which are not unique to past discussions), I'm really busy today with work so I can't afford a lengthy back and forth on this.

revero-doug commented 1 year ago

lol perfectly fitting you just responded with the equivalent of "works on my machine". this library is a virus and the disease is dependency hell that goes way beyond version conflicts.

you are trying to bully us to completely change the direction of the project

the fact that you see ESM purity test as your "vision" for this project is hilarious. listen to your users, and no I won't be counted among them. the only vocal plurality on this topic that I've seen pouring through issues the past two days to get this to play nicely in a young project with few dependencies is firmly against your obsession with ESM purity. saying "it's my ball, I'm taking it home" is a lot closer to bullying than calling out said behavior.