fastify/fastify-request-context

Using `strict` in a project `tsconfig.json` makes `requestContext.get(string)` possibly `undefined`

silenceisgolden opened this issue · 5 comments

silenceisgolden commented

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the bug has not already been reported

Fastify version

3.25.0

Plugin version

2.2.0

Node.js version

16.x

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

12.0.1 Monterey

Description

With a project using fastify and fastify-request-context, the user could get confused because

declare module 'fastify-request-context' {
    interface RequestContextData {
       a: number;
    }
}

const value = request.requestContext.get('string');

and value is set to number | undefined. In the documentation it states that the value should be set to a type of number, but this is not always the case.

Steps to Reproduce

Create a new folder/nodejs project with these files.

package.json

{
  "name": "fastify-request-context-typescript",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "keywords": [],
  "author": "",
  "license": "ISC",
  "dependencies": {
    "fastify": "^3.25.0",
    "fastify-request-context": "^2.2.0"
  },
  "devDependencies": {
    "@types/node": "^17.0.0"
  }
}

tsconfig.json

{
    "compilerOptions": {
        "strict": true // the cause of the issue
    }
}

app.ts

import fastify, { FastifyLoggerInstance } from 'fastify';
import { fastifyRequestContextPlugin } from 'fastify-request-context';

import { simpleController } from './a';

declare module 'fastify-request-context' {
    interface RequestContextData {
        logger: FastifyLoggerInstance;
    }
}

const server = fastify();

server.register((app) => {
    app.get('/', {
        handler: simpleController
    });
}, {
    prefix: '/helloworld',
})

server.register(fastifyRequestContextPlugin, {
    defaultStoreValues: {
        logger: server.log,
    },
});

a.ts

import { RouteHandler } from 'fastify';

export const simpleController: RouteHandler = async (req, reply) => {
    const logger = req.requestContext.get('logger');
    
    // logger is possibly undefined, so this is a typescript error
    logger.info('do a thing');

    reply.code(204);
};

The issue is due to this typing I think:

https://github.com/fastify/fastify-request-context/blob/master/index.d.ts#L8

Since the type is RequestContextData[K] | undefined, in strict typing mode I think that this causes logger above in a.ts to be FastifyLoggerInstance | undefined. This is not the case if you add "strictNullChecks": false to the tsconfig.json.

Expected Behavior

Could the .get method just be typed as get<K extends keyof RequestContextData>(key: K): RequestContextData[K]? This seems to work with "strictNullChecks" set to either true (implicitly by "strict": true), or with "strict": true and "strictNullChecks": false.

silenceisgolden commented

Thanks for the quick response! So that sort of tells me that the docs should at least be updated then? https://github.com/fastify/fastify-request-context/blob/master/README.md where it says

// Type is string
const foo = requestContext.get('foo')
// Type for unspecified keys is any
const bar = requestContext.get('bar')

maybe there could be a comment added that this is only the case when "strictNullChecks": false?

climba03003 commented

Would you like to send a Pull Request to improve the documentation?

  • 👍1
silenceisgolden commented

For other folks who run into this issue, I am sort of fixing this by redefining the type:

declare module 'fastify-request-context' {
  interface RequestContextData {
    val: string;
    val2?: string;
  }

  interface RequestContext {
    get<K extends keyof RequestContextData>(key: K): RequestContextData[K];
  }
}

I think this is valid because I am setting correctly typed defaults:

import fastify from 'fastify';
import { fastifyRequestContextPlugin } from 'fastify-request-context';

const server = fastify();

server.register(fastifyRequestContextPlugin, {
  defaultStoreValues: {
    val: '',
    // Since `val2` is typed as possibly undefined, we can leave it undefined here.
  },
});

If you decide to set defaultStoreValues with this type fix, you should be fine since TS will error until you get the correctly typed defaults into the defaultStoreValues object for each property. However, if you do not set defaultStoreValues, you can have a situation where the types are telling you that the value will always be defined, but in fact start out as undefined.

silenceisgolden commented

Closing this issue now that the pull request has been merged and it is link to from the README. Please ping me on this thread though if anyone has a better idea of how to work around this issue. Thanks @climba03003 for the discussion.

  • ❤️1