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

Support node 17 & typescript #306

Closed meabed closed 2 years ago

meabed commented 2 years ago

Thank you for making this 🙏

First issue:

In the last version you have add node18 support, but there is node 17 as well :) It would be great to add this to the package json:

error graphql-upload@14.0.0: The engine "node" is incompatible with this module. Expected version "^14.17.0 || ^16.0.0 || >= 18.0.0". Got "17.9.0"

Second issue:

The typescript "@types/graphql-upload": "8.0.11" is not incompatible with the package, it's going to very helpful if this is updated or published the types in the package it self.

Old version import 13

import { GraphQLUpload } from 'graphql-upload';
import { graphqlUploadExpress } from 'graphql-upload';
import { FileUpload } from "graphql-upload";

New version import 14

import GraphQLUpload from 'graphql-upload/GraphQLUpload.js';
import graphqlUploadExpress from 'graphql-upload/graphqlUploadExpress.js';
import { FileUpload } from "graphql-upload"; // not existing
jaydenseric commented 2 years ago

Thanks for the thanks, glad to help :)

First issue:

In the last version you have add node18 support, but there is node 17 as well :)

In just a week (2022-06-01), Node.js v17 reaches end-of-life:

https://nodejs.org/en/about/releases/

It's not supported for the same reason Node.js v15 isn't supported.

Even if we wanted to support it, pretty soon we can't anyway as dependencies and dev dependencies drop the end-of-life Node.js v17 from their support, for example graphql v17:

https://github.com/graphql/graphql-js/blob/c7d7026982ceee536900a24ae31235127560297a/package.json#L27-L29

By getting ahead of that in this major release we can later update dependencies that drop Node.js v17 more easily without having to publish a new major version or document it in the changelog entry under major breaking changes, which is distracting even if we needed to publish a major version for other reasons.

It seems like you are combining 2 things in your second point of feedback? Regarding @types/graphql-upload, you should uninstall that from your projects as it's redundant now types are published within graphql-upload. To make use of them properly, see these tips:

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

Regarding the package main index module being removed and import paths changing, that is on purpose and is documented in the graphql-upload v14 changelog entry under major changes. You can see an explanation for this change here:

https://github.com/jaydenseric/graphql-upload/issues/305#issuecomment-1134736158

The types can be imported (via TypeScript type imports, e.g. within JSDoc types) directly from the specific modules they originate from. For example:

https://github.com/jaydenseric/apollo-upload-examples/blob/86e5609e60d86a92fd6060679641105beec715e5/api/storeUpload.mjs#L10-L12

Note that the TypeScript types published in graphql-upload have been created from scratch by me, and are not intended to match exactly whatever types previously existed in @types/graphql-upload.

meabed commented 2 years ago

Thank you so much for the reply.

I got your point for node version, My suggestion is there is no need to limit the support for the engine unless the engine break compatibility with the lib apis. Just like all other libs graphql, etc... but it's your choice.

For the type script, maybe it would handy and easier if there is declaration type published and declared in package json as well, just like the ones you have inside - it makes typescript imports a lot easier - if possible.

Thank you again for the effort, discussion and reply 🙏