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

This module cannot be used from commonjs #354

Closed peterhoneder closed 1 year ago

peterhoneder commented 1 year ago

Hi!

I read your previous comments, but they make no sense to me, since you disabled an upgrade path for all kinds of reasons including security updates for anyone using an older version of your library.

Also importing it as a dynamic import from commonjs is no option for anything that more or less declaratively uses GraphQLUpload in a schema. I think there should at least be a compatibility workaround for those deployments.

ZenSoftware commented 1 year ago

Would it be possible ship a .cjs variant alongside the new .mjs? I suspect it will be rather confusing to newcomers to have to install version 15 to work with CommonJS. The majority of the industry will continue to use CommonJS for a very long period of time. This will really hurt the adoption of this library.

jaydenseric commented 1 year ago

@ZenSoftware shipping CJS and ESM variants of the same modules together in one package is an anti pattern that leads to the dual package hazard; I attempted to do it for years and it causes problems.

@peterhoneder migrate your codebase to ESM and you can use CJS or ESM dependencies as you please.

All of my packages (with the exception of plugins for things that require a specific format) will be only web standard ESM moving forwards. I've discussed the reasons why many times in past issues in various repos. I'm not the only package author with this policy, so it's not realistic for people to expect CJS codebases to be viable moving forwards. The technical debt of a codebase written in a non web standard CJS format lies with the employees paid to maintain legacy codebases, not with unpaid open source package authors. I can't publish packages that both work for legacy CJS projects and run natively in web browsers, Deno etc. at the same time, so of course I'm going to choose web standards and what works everywhere in browsers and things. In my own projects I use ESM in the browser and in Deno without bundling.

ZenSoftware commented 1 year ago

I beg to differ. Shipping both is not an anti-pattern. It's considered good practice for maximizing compatibility. For example, Apollo v4 ships with both ECMAScript or CJS modules. I too wish that there wasn't such confusion and incompatibility with the JavaScript module system. Though practically speaking, too much is still dependent on CJS.

For those looking for alternatives, graphql-upload-minimal is a fork of this and has some advantages over this package. It works with CJS.

jaydenseric commented 1 year ago

I beg to differ. Shipping both is not an anti-pattern. It's considered good practice for maximizing compatibility.

Consider that the original react package is CJS. Now imagine someone forked it and converted every module into ESM and published it as fork-of-react-that-is-esm. You would intuitively understand it would be a bad idea to install both in your project dependencies and in some components import this:

import { useState } from "react";

And in other components import:

import { useState } from "fork-of-react-that-is-esm";

For the obvious reason that they are two different packages, and you will end up with two completely different useState implementations bundled in your app (bloating bundle size), and it would probably corrupt your app rendering since you are rendering with one package while using features from another. What you need to do, is only install one of those dependencies in your project and make sure you only import from that one in all your project code, and also make sure that any third party components also only import from your preferred React package.

Now imagine if that fork of react's ESM modules were copy pasted into the same react dependency directory in node_modules. It's the same problem as before, even if the files happen to be contained in one package instead of two; you need to make sure that 100% of the code in your app uses only the CJS or the ESM version, but not both. So say you are absolutely sure that your project and its dependencies only imports from the ESM version; why would you want all the CJS modules you are not using to be bloating your node_modules? It would be preferable to just have a package in the format you want without the bloat of the format you don't.

Having CJS and ESM clones of the same features in one package implies sometimes the project will use one format, and other times the other. This is fundamentally bad and not desirable at all. I don't want to publish seperate ESM and CJS versions of graphql-upload because what immediately happens is a tangled web of an ecosystem where some packages import the ESM, others the CJS, and you can't use them together in the same app. This is not a theoretical concern, but a lived experience. graphql-upload will not work due to failed instanceof checks if your processRequest function in middleware comes from one format but the GraphQLUpload scalar and Upload class came from another format. Maybe your project code is ESM and you import the GraphQLUpload scalar when setting up your resolvers, but you installed a GraphQL server that automatically setup the scalar or middleware that required the CJS version of graphql-upload. Now you have a mess that won't function.

There is absolutely no reason the ecosystem can't all be ESM; the web standard format. Publishing CJS just enables the same people responsible for making all of this way more complicated than it needs to be.

ZenSoftware commented 1 year ago

I understand your concern. Though, why do popular libraries like Apollo Server provide both variants then? It is true you should not mix their usage. I don't think publishing both implies you can use both in the same project. I think that is common knowledge that would simply cause problems.

The reason I am pushing for this is that I am using Nx as a monorepo. Nx has longstanding issues with ESM. I and others simply want CJS for compatibility reasons. until we are unblocked from upgrading to ESM.

There are simple ways to maintain both variants by utilizing something like Babel or TypeScript. This is your library though, so all I can do is ask nicely. Thank you for your time and effort. Open source is not easy...

eduardolundgren commented 10 months ago

@jaydenseric It appears that you have reasons for not publishing CJS, but you would be assisting an entire community by making this library available. While the ultimate goal of using ESM is commendable, supporting CJS in the interim would greatly benefit many people.