Implement TypeScript types

[saihaj]

It would be better if TypeScript types were exported from this package. As a user it simplifies using this library and as an author that should make it easier to supporting latest versions. Let me know and I can help getting TypeScript types from Definitely typed to this package.

[yaacovCR]

Relevant: jaydenseric/apollo-upload-client#267 (comment)

[jaydenseric]

Eventually there will be types in this package, but they will also be Deno compatible, and most likely JSDoc types that will be defined in the actual published code and will also be used when generating the readme API docs.

I've been doing intense research and development over the past 6 months in particular about a universal pattern for type safe Node.js/Deno/browser/JSPM code, that is buildless and works for pure ESM modules (.mjs). We're getting close, but there are some blockers. The Node.js flavour of TypeScript and their lack of support for Node.js ESM had fixes that were going to ship in typescript v4.5, but now will hopefully come in v4.6. There are a few Deno bugs around JSDoc types that I hope will get resolved sometime soonish.

I've figured out how to get universal Node.js/Deno/JSPM packages working for everything except types, and am currently republishing everything accordingly. I'll do another wave of updates once types are possible.

[glennreyes]

@jaydenseric Could we release another version with the TS types? Or anything you would like to wait with along the next bump?

[jaydenseric]

@glennreyes I really wanted to update the busboy dependency to v1 (it contains some fantastic improvements) for this coming major release, but unfortunately that's been blocked by mscdex/busboy#297 . I was hoping it would be solved within a week or so, but the maintainer has ignored the issue for close to a month now :(

Perhaps if you were to figure out a PR there it would expedite things? It would be a good way to help.

I'm currently updating the Apollo upload examples project so I can test the graphql-upload major changes before releasing it most likely close to how it is now in master branch. Dependencies need updating, modernise the tooling, and then attempt to add type safety so the new graphql-upload types can be tested in an actual project.

I was considering also switching graphql-upload to pure ESM and updating the fs-capacitor dependency for this release, but I think it might be better to wait for the pure ESM graphql v17 stable release. Also, people who for whatever reason are unwilling to update their projects to ESM will probably appreciate a final CJS release.

[jaydenseric]

Types have been published in v14.0.0 🚀

If you wish to make use of them in your Node.js projects, consider using these TypeScript config options:

graphql-upload/jsconfig.json

Lines 3 to 4 in e682fb9

	"maxNodeModuleJsDepth": 10,
	"module": "nodenext",

You will also need to use a TypeScript version that supports those options.

[Bidek56]

Any idea how to resolve this TS error when using 15.0.0, thx

server.ts:7:34 - error TS7016: Could not find a declaration file for module 'graphql-upload/graphqlUploadExpress.js'. '/Users/name/git/data-profiler/api/node_modules/graphql-upload/graphqlUploadExpress.js' implicitly has an 'any' type.

import graphqlUploadExpress from 'graphql-upload/graphqlUploadExpress.js';

[jaydenseric]

@Bidek56 have you tried these tips: #282 (comment)

Are you importing graphql-upload/graphqlUploadExpress.js in your project code? If not, it's a little tricker to deal with. The reason I ask is because sometimes when you TypeScript type check all the files in your project it can enter node_modules and have errors for things you didn't import yourself.

[Bidek56]

Are you importing graphql-upload/graphqlUploadExpress.js in your project code? If not, it's a little tricker to deal with. The reason I ask is because sometimes when you TypeScript type check all the files in your project it can enter node_modules and have errors for things you didn't import yourself.
yes, I am importing it in my server.ts file using:
import graphqlUploadExpress from 'graphql-upload/graphqlUploadExpress.js';
Thanks for your help!!

[jaydenseric]

@Bidek56 are you using the latest version of TypeScript, and have you tried the TypeScript config options explained here?

#282 (comment)

allowJs might be another option to try fiddling with, although I haven't needed to use it in my projects.

[Bidek56]

So it turns out that tsconfig.json "noImplicitAny": true is causing this error:
Could not find a declaration file for module 'graphql-upload/graphqlUploadExpress.js'. '/Users/name/git/graphql-upload-try/node_modules/graphql-upload/graphqlUploadExpress.js' implicitly has an 'any' type.

Changing it to: "noImplicitAny": false fixes the error.
Thanks for your help.

[jaydenseric]

@Bidek56 I don't think that fixes the problem of the types not resolving correctly; it just hides that they are missing.

[Bidek56]

Hmmm, maybe it's a ts-node problem b/c it works with tsc index.ts but it does not when using ts-node index.ts
on a simple index.ts: import graphqlUploadExpress from 'graphql-upload/graphqlUploadExpress.js';

[Bidek56]

It's a ts-node config problem. When using ts-node, the tsconfig.json needs to be:

{
    // Most ts-node options can be specified here using their programmatic names.
    "ts-node": {
      // It is faster to skip typechecking.
      // Remove if you want ts-node to do typechecking.
      "transpileOnly": true,
      "files": true,
      "compilerOptions": {
        // compilerOptions specified here will override those declared below,
        // but *only* in ts-node.  Useful if you want ts-node and tsc to use
        // different options with a single tsconfig.json.
      }
    },
  "compilerOptions": {
    "module": "NodeNext",
    "maxNodeModuleJsDepth": 10
  },
  "exclude": [
    "dist",
    "node_modules"
  ],
  "include": [
    "./*.ts"
  ]
}

[ghost]

Is there any other solution for this problem if i don't want to use unstable NodeNext with typescript@next?

[jaydenseric]

@dzyubanov it's not unstable, it's stable in TypeScript v4.7:

• https://devblogs.microsoft.com/typescript/announcing-typescript-4-7/#esm-nodejs
• https://github.com/microsoft/TypeScript/releases/tag/v4.7.2

[werkshy]

@Bidek56 are you using the latest version of TypeScript, and have you tried the TypeScript config options explained here?

#282 (comment)

allowJs might be another option to try fiddling with, although I haven't needed to use it in my projects.

I mentioned this on another issue but I did need to set allowJs: true in my Typescript project to make the new module settings work with graphql-upload.

[Bidek56]

I am using TS 4.7.2 but it was an issue with ts-node solved by adding to tsconfig.json as shown above.
Thanks for your help.

[jaydenseric]

Regarding allowJs: true, you might need that if you are using tsconfig.json; if you are using jsconfig.json TypeScript automatically sets allowJs to true for you:

https://code.visualstudio.com/docs/languages/jsconfig#_what-is-jsconfigjson

I use jsconfig.json in my projects because I only use JavaScript files (.js/.mjs) and don't use .ts/.mts/.cts with a compile step.

[glennreyes]

@jaydenseric Any strong reasons why we don't want this library to ship types that just work with a tsconfig.json without setting allowJs: true?

If yes, I think this would need definitely some docs.

[mlesin]

I took source of this library, changed jsconfig.json to include these lines:

"noEmit": false,
"emitDeclarationOnly": true,
"declaration": true,

and after that step tsc -p jsconfig.json generated pretty useful type declarations allowing me to avoid using "allowJs": true and "checkJs": true in my tsconfig.json

Can I hope something like this could happen during publishing of this library?

Reason it's pretty important for me, is in my project, setting "allowJs": true and "checkJs": true in my tsconfig.json starts to cause lots of problems from other third-party libraries, like circular dependencies from class-validator and so on.
On the other hand, if I don't set these vars, everything compiles and works, but no types are checked from graphql-upload library, which is also bad for me.
So I hope there is some option exists to include these generated types I described above into the package by default.

[jaydenseric]

@glennreyes yes, there are several very strong reasons. One that is easy to explain, is that when a Deno user imports a module from a simple CDN that just dumbly serves the package files that were published, the types will be perfectly available without having to do anything other than importing the module. For example:

import Cache from "https://unpkg.com/[email protected]/Cache.mjs";

Some other benefits (not the only ones):

• No build step for the published package. This reduces complexity, makes it easier to work on, and less chance of making mistakes as the code and types you use in the package tests is the same types that your consumers will use when importing the package. My quality of life as a package maintainer has dramatically improved since removing build steps from packages.
• Less published files, meaning a substantially smaller package install size. Everyone complains about how bloated node_modules is on their disks; well here is one of the concrete ways we can improve that situation.

It would take a much longer comment to explain the downsides to alternative approaches like triple slash reference comments in the published modules, or @deno-types comments in the consuming modules. At the end of the day, fully self contained standard JavaScript modules that also contain all the types that can run in all the Node.js, Deno, and browser runtimes without a build step are highly desirable. It just works in Deno, and Node.js TypeScript uses only have to minimally adjust their TypeScript config to enjoy the same benefits.

graphql-upload is not yet ESM or Deno compatible (in the future it will be), but many of my published packages already are and are being actively used in type safe Deno projects. I maintain quite a few packages, and am rolling out a universal buildless standard JavaScript approach for all of them.

Regarding docs, thanks for the suggestion but I disagree that the burden is on every individual package to teach users how to use tools like TypeScript. I would be copy-paste synchronising teaching material across something like 20+ readmes. And all of that content would be irrelevant to Deno users anyway, as they don't need to do any kind of special config to enjoy types from JSDoc comments in JavaScript modules. Node.js TypeScript users need to be aware that there are a few different ways that packages provide types, and not set TypeScript config that excludes some.

I'm getting more complaints in the few days I added types here than in the years I provided none at all, and I'm not doing anything invalid. They are good types!

[jaydenseric]

@mlesin "checkJs": true should not be used, for the reasons you are discovering. I don't use it in my projects either; if you want specific modules in your project to be type checked add a // @ts-check comment at the top like this:

graphql-upload/GraphQLUpload.js

Line 1 in b1cdd2a

// @ts-check

[mlesin]

@jaydenseric I tried hard and still can't find variant of tsconfig.json where I would see types from these import expanded without putting at least "allowJs": true" in it. Maybe you could point what I am doing wrong?
Here is my tsconfig.json (it's nestjs project), vscode uses typescript from node_modules (4.7.2):

{
  "compilerOptions": {
    "module": "NodeNext",
    "maxNodeModuleJsDepth": 10,
    "declaration": true,
    "removeComments": true,
    "emitDecoratorMetadata": true,
    "experimentalDecorators": true,
    "allowSyntheticDefaultImports": true,
    "lib": ["ESNext", "DOM"],
    "target": "ESNext",
    "sourceMap": true,
    "outDir": "./dist",
    "baseUrl": "./",
    "incremental": true,
    "skipLibCheck": true,
    "strict": true,
    "strictNullChecks": true,
    "noImplicitAny": false,
    "strictBindCallApply": false,
    "forceConsistentCasingInFileNames": false,
    "noFallthroughCasesInSwitch": false,
    "noUncheckedIndexedAccess": true
  }
}

I am importing library this way:

import GraphQLUpload from 'graphql-upload/GraphQLUpload.js';
import type { FileUpload } from 'graphql-upload/processRequest.js';

vscode writes this under first quote right after "import GraphQLUpload from":

Could not find a declaration file for module 'graphql-upload/GraphQLUpload.js'. '/workspace/backend/node_modules/graphql-upload/GraphQLUpload.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/graphql-upload` if it exists or add a new declaration (.d.ts) file containing `declare module 'graphql-upload/GraphQLUpload.js';`ts(7016)

And as a result, GraphQLUpload is of type any.
What should I do to fix that?

[jaydenseric]

@mlesin if you are using tsconfig.json and not jsconfig.json, then "allowJs": true is necessary. See this comment:

#282 (comment)

TypeScript is really not helpful with their error messages in some of these situations; hopefully in time they will fix them because Could not find a declaration file for module is very misleading. There are several situations it will say that when declaration files have got nothing to do with the problem. For example, if you have a JSDoc typed dependency, that in turn imports another JSDoc typed dependency of it's own, and the maxNodeModuleJsDepth is set to the default of 0, TypeScript will on purpose stop looking at any types that come from modules deeper than the first one. In that situation it says Could not find a declaration file for module instead of something like Types ignored because TypeScript config option `compilerOptions.maxNodeModuleJsDepth` of 0 (the default) has been reached (it could be even better than that, by including the layers of the branch of the module graph in the message and a suggestion for what value for maxNodeModuleJsDepth would fix it).

[renarsvilnis]

The "cleanest" solution found (still you lose type safety) is to import the .js file and tell typescript that you are expecting an error via @ts-expect-error which allows it to compile without disabling any noImplicitAny or other strict checks

// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-expect-error See Github issue why this exists - https://github.com/jaydenseric/graphql-upload/issues/305#issuecomment-1135285811
import GraphQLUpload from "graphql-upload/GraphQLUpload.js";

The same goes for the middleware as well. Type safety can still be opted-in if you really need it by import type {...} from 'graphql-upload'; as import type still works!

[Faithfinder]

Ok, I guess I'm screwed if I don't want to allow JS in my project, eh? Let us all sacrifice for the Deno people

[jaydenseric]

@Faithfinder firstly, why is it a big sacrifice to enable allowJs in your project? How are you "screwed" if you enable it? A lot of people already have it enabled; for example it was already enabled in the Node.js TypeScript projects I'm currently doing contract work for.

Secondly, scorning Deno and browser people is not productive and is going to age like milk.

[Faithfinder]

Sorry about being.... Unfriendly. I was just upset having to dive through these topics, first about no main field and then about Typescript. I would say that shipping .d.ts files and having an index export is the modern-day convention and 99% of the packages I have to use follow it.
I sorta got over it? The only type from the library you really need is FileUpload, which can be redefined locally easily enough.
For GraphQLUpload any works just fine because you feed it to your GQL implementation and forget, and same for middleware, so a declaration file like declare module 'graphql-upload/GraphQLUpload.js'; "fixes" the situation.

Still.
For allowJs it's more of a principle thing than a real concern. allowJs means that js is, well, allowed in the project. It is not. I don't want a single file of JS source code and the flag enforces that.

maxNodeModuleJsDepth is much worse. First of all, there's a warning that it affects speed (of typecheck, I assume). I don't want to take a hit to my build times.

Ideally this should stay at 0 (the default), and d.ts files should be used to explicitly define the shape of modules. However, there are cases where you may want to turn this on at the expense of speed and potential accuracy.

Secondly, I actually don't want types inferred for random libraries in node modules. Sure, you might be shipping JSDoc types, however most libraries if not shipping .d.ts won't have types defined at all, meaning that they would be inferred, which is not very precise (for example, inference treats parameters without defaults as required, which is usually not true).

As for Deno... It might be the future or whatever, however we live in the present. Some future (not even current) support for Deno does not make me feel like I spent my time productively reading multiple issues where you define your stance. Fact is, in the current ecosystem, after you download the package you either get the types working automatically, or you download the @types/, no need to mess with configs. To use this package, you instead need to research what went wrong. I'm 100% certain that most engineers, however seasoned they are, won't be able to set this up without going "WTF?"

[saturn72]

Have 6 projects in production using this feature
Changing the import may be correct for the mid/long term, but cause us, the users, major issues.

Perfoming this change without publicly notify it nor enable well defined migration path is must in these cases.

This "publishing-behavior" causes instability and breaks the trust we give to the library.

For these we are dropping the usage of this package.
Thanks for the contribution!

[ash-r1]

Note that graphql-upload-ts or https://github.com/flash-oss/graphql-upload-minimal could be a solution for us(TypeScript users, using cjs for now).