Improve typescript types

[StarpTech]

🚀 Feature Proposal

• Some interfaces aren't covered e.g onFile handler in the new promise API.
• Validate TS types in the CI.

Motivation

Full TS support.

[nadhifikbarw]

Would maintainer okay with the idea of adding one extra property? I'm thinking type with literal string as value, for both MultipartFile with value ("file") and MultipartValue with value ("field") to make type narrowing feel more intuitive.

Use case: https://github.com/fastify/fastify-multipart#handle-multiple-file-streams-and-fields

fastify.post('/upload/raw/any', async function (req, reply) {
  const parts = req.parts()
  for await (const part of parts) {
    if (part.file) {  // Failed type narrowing
      await pump(part.file, fs.createWriteStream(part.filename))
    } else {
      console.log(part)
    }
  }
  reply.send()
})

However

fastify.post('/upload/raw/any', async function (req, reply) {
  const parts = req.parts()
  for await (const part of parts) {
    if ("file" in part) {  // Success type narrowing
      await pump(part.file, fs.createWriteStream(part.filename))
    } else {
      console.log(part)
    }
  }
  reply.send()
})

I feel like having type property would feel more intuitive. It would look like:

fastify.post('/upload/raw/any', async function (req, reply) {
  const parts = req.parts()
  for await (const part of parts) {
    if (part.type === 'file') {  // Using discriminated unions
      await pump(part.file, fs.createWriteStream(part.filename))
    } else {
      console.log(part)
    }
  }
  reply.send()
})

Alternative, expose isFilePart as type predicate, but i felt this bit too much.

function isFilePart(part: Multipart): part is MultipartFile {
  return (part as MultipartFile).file !== undefined;
}

fastify.post('/upload/raw/any', async function (req, reply) {
  const parts = req.parts()
  for await (const part of parts) {
    if (isFilePart(part)) {  // Using discriminated unions
      await pump(part.file, fs.createWriteStream(part.filename))
    } else {
      console.log(part)
    }
  }
  reply.send()
})

[mcollina]

Sure!

[jsjoeio]

I spent a lot of time trying to make TypeScript to be happy with me using req.file in my router handler. The solution ended up being adding fastify.d.ts with this:

import { Readable } from "stream";
declare module "fastify" {
  export interface FastifyRequest {
    file: () => Promise<{
      file: Readable;
      fieldname: string;
      filename: string;
      encoding: string;
      mimetype: string;
      fields: { [key: string]: string };
    }>;
  }
}

and then including that file in my typeRoots in my tsconfig.json:

{
  "compilerOptions": {
    "typeRoots": ["fastify.d.ts"]
  },
}

Posting here in case it helps others.

[gurgunday]

Would you like to send a PR?

@jsjoeio

[jsjoeio]

Sure! I'll see if I can find time this week or next!

[jsjoeio]

Okay I started looking into this and I'm very confused 🤔

Here's a repo.

I thought by installing @fastify/multipart and then registering it like so:

const server = fastify();

server.register(require("@fastify/multipart"));

then TypeScript would pick up the library's types, but if you look here, it doesn't have isMultipart for example (here)

So I'm not really sure how to fix (without digging in further). Anyone else have any ideas on what I'm doing wrong? Here's the commit adding multipart

[mcollina]

Import the module at the top, otherwise TS won't load the declaration merging.

[jsjoeio]

Import the module at the top, otherwise TS won't load the declaration merging.

😅 wow...sorry, totally spaced on that. that fixed and i'm no longer able to reproduce the issue I had before:

diff --git a/index.ts b/index.ts
index 18fe828..9dfc3c7 100644
--- a/index.ts
+++ b/index.ts
@@ -1,4 +1,5 @@
 import fastify from "fastify";
+import "@fastify/multipart";
 
 const server = fastify();
 
@@ -13,7 +14,7 @@ server.post("/upload", async function (req, reply) {
     const data = await req.file();
 
     reply.status(200).send({
-      fileName: data.filename,
+      fileName: data?.filename,
     });
   } catch (error) {
     reply.status(400).send("something went wrong");

@gurgunday should we close this then, or did you have something else in mind for the PR?

[gurgunday]

I see in the issue description that onFile doesn’t have corresponding type information

Can you please try adding it?

[jsjoeio]

I probably won't get to this so anyone else, feel free to take on instead