Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

eslint no-misused-promise in notFoundHandler #355

Closed
2 tasks done
mindrunner opened this issue Jan 3, 2024 · 17 comments · Fixed by fastify/fastify#5258
Closed
2 tasks done

eslint no-misused-promise in notFoundHandler #355

mindrunner opened this issue Jan 3, 2024 · 17 comments · Fixed by fastify/fastify#5258
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@mindrunner
Copy link

Prerequisites

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

Fastify version

4.25.2

Plugin version

9.0.1

Node.js version

20.10.0

Operating system

Linux

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

Description

I am rate limiting 404 errors as documented here:
https://github.com/fastify/fastify-rate-limit#preventing-guessing-of-urls-through-404s

notFoundHandler expects a preValidationHookHandler, but fastify.rateLimit() is a preHandlerAsyncHookHandler

Steps to Reproduce

Use

fastify.setNotFoundHandler({
  preHandler: fastify.rateLimit()
}, function (request, reply) {
  reply.code(404).send({ hello: 'world' })
})
  • Enable no-misused-promise eslint rule

Expected Behavior

No erorrs / warnings thrown

@mcollina
Copy link
Member

mcollina commented Jan 3, 2024

Fastify normalizes the flow between promises and non-promisified code.

Essentially that rule is too strict for no specific reason. Reply has a .then() but it is not a Promise.

To solve, just add a void, like so:

fastify.setNotFoundHandler({
  preHandler: fastify.rateLimit()
}, function (request, reply) {
  void reply.code(404).send({ hello: 'world' })
})

or return it:

fastify.setNotFoundHandler({
  preHandler: fastify.rateLimit()
}, function (request, reply) {
  return reply.code(404).send({ hello: 'world' })
})

@mcollina mcollina closed this as completed Jan 3, 2024
@mindrunner
Copy link
Author

The problem here is not the handler itself, but the preHandler which does not match:

image

@mcollina mcollina reopened this Jan 3, 2024
@mcollina
Copy link
Member

mcollina commented Jan 3, 2024

Ah it might be a bug in our types. That does not return a promise. That returns a function that returns a promise.

@mcollina mcollina added bug Something isn't working good first issue Good for newcomers labels Jan 3, 2024
@mcollina
Copy link
Member

mcollina commented Jan 3, 2024

Would you like to send a PR?

@mindrunner
Copy link
Author

Yea that was my assumption. Can have a look into this later.

@mindrunner
Copy link
Author

mindrunner commented Jan 6, 2024

I am not sure how to fix this. preHandler expects a preHandlerHookHandler, but rateLimit returns a preHandlerAsyncHookHandler

@mcollina
Copy link
Member

mcollina commented Jan 6, 2024

Can you share a reproduction for this? So somebody can quickly work for a resolution.

@mindrunner
Copy link
Author

See 'Steps to reproduce' in Original Post. What else would you need to reproduce?

@mcollina
Copy link
Member

mcollina commented Jan 7, 2024

We often need a reproducible example, e.g. some code that allows someone else to recreate your problem by just copying and pasting it. If it involves more than a couple of different file, create a new repository on GitHub and add a link to that.

TL;DR Set up a tiny repository that shows the problem.

@mindrunner
Copy link
Author

mindrunner commented Jan 7, 2024

app.ts:

import Fastify from "fastify";

const main = async () => {
  const fastify = Fastify();
  await fastify.register(import("@fastify/rate-limit"), {
    max: 100,
    timeWindow: "1 minute",
  });

  fastify.setNotFoundHandler(
    {
      preHandler: fastify.rateLimit(),
    },
    function (request, reply) {
      reply.code(404).send({ hello: "world" });
    },
  );
};

main().then(() => {});

package.json

{
  "name": "rl-fastify",
  "version": "1.0.0",
  "description": "",
  "dependencies": {
    "fastify": "^4.25.2",
    "@fastify/rate-limit": "^9.1.0"
  },
  "devDependencies": {
    "@typescript-eslint/eslint-plugin": "^6.16.0",
    "@typescript-eslint/parser": "^6.16.0",
    "eslint": "^8.56.0"
  }
}

tsconfig.json

{
  "compilerOptions": {
    "target": "es2016",
    "module": "commonjs",
    "esModuleInterop": true,
    "forceConsistentCasingInFileNames": true,
    "strict": true,
    "skipLibCheck": true
  },
  "include": [
    "app.ts",
    ".eslintrc.js"
  ]
}

.eslintrc.js

module.exports = {
  parser: "@typescript-eslint/parser",
  parserOptions: {
    sourceType: "module",
    project: ["./tsconfig.json"],
  },
  env: {
    es6: true,
    node: true,
  },
  rules: {},
  extends: ["plugin:@typescript-eslint/recommended"],
};

Steps to reproduce:

  • copy all 4 files to same directory
  • run pnpm install
  • run eslint .

Actual output:

  12:19  error  Promise-returning function provided to property where a void return was expected  @typescript-eslint/no-misused-promises

✖ 1 problem (1 error, 0 warnings)

Expected output:

<empty> (e.g. eslint does not show any errors)

@mcollina
Copy link
Member

mcollina commented Jan 7, 2024

I think you got your file names wrong. I recommend you to upload everything to a repository.

@mindrunner
Copy link
Author

Sorry, accidentally hit save before I was ready.

@mindrunner
Copy link
Author

@mcollina
Copy link
Member

mcollina commented Jan 8, 2024

Running npx eslint . in that repository does not trigger any errors.

@mindrunner
Copy link
Author

updated repo. pls try again

@mcollina
Copy link
Member

mcollina commented Jan 8, 2024

Thanks, that was helpful.

Here is the fix: fastify/fastify#5258

@mindrunner
Copy link
Author

Awesome, thanks for fixing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants