Skip to content

types: add string[] to routeOptions.method#5762

Merged
mcollina merged 2 commits into
fastify:mainfrom
smartinio:fix/routeOptions-method-type
Oct 24, 2024
Merged

types: add string[] to routeOptions.method#5762
mcollina merged 2 commits into
fastify:mainfrom
smartinio:fix/routeOptions-method-type

Conversation

@smartinio

@smartinio smartinio commented Oct 23, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

The type for routeOptions.method is incorrect and may cause runtime errors if assumed to be a string.

it('is lying', async () => {
  const server = fastify();

  server.route({
    method: ['HEAD', 'GET'],
    url: '/docs',
    handler: async (request, reply) => {
      await reply.send({
        method: request.routeOptions.method, // typed as string
      });
    },
  });

  const response = await server.inject({
    method: 'GET',
    url: '/docs',
  });

  expect(response.statusCode).toBe(200);
  expect(response.json().method).toBe('GET'); // received: ["HEAD", "GET"]
});

Checklist

@github-actions github-actions Bot added the typescript TypeScript related label Oct 23, 2024
@smartinio smartinio marked this pull request as ready for review October 23, 2024 08:05
climba03003

This comment was marked as duplicate.

Sign in to view

climba03003
climba03003 requested changes Oct 23, 2024
View reviewed changes

@climba03003 climba03003 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add an unit test for the types change.

@climba03003 climba03003 changed the title Add string[] to routeOptions.method types: add string[] to routeOptions.method Oct 23, 2024
@smartinio smartinio requested a review from climba03003 October 23, 2024 09:39
@smartinio

smartinio commented Oct 24, 2024

Copy link
Copy Markdown
Contributor Author

@climba03003 Is anything else needed from me here to get this merged?

@Uzlopak

Uzlopak commented Oct 24, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

You could actually improve the developer experience significantly.

add following type to utility.d.ts, see like nodejs/undici#3479

type AutocompletePrimitiveBaseType<T> =
  T extends string ? string :
  T extends number ? number :
  T extends boolean ? boolean :
  never;

export type Autocomplete<T> = T | (AutocompletePrimitiveBaseType<T> & Record<never, never>);

Also export _HTTPMethods in utility.d.ts

In request.d.ts modify you change by changing the type of method string | string[] to (AutoComplete<_HTTPMethods> | AutoComplete<_HTTPMethods>[])

Cross check if at runtime the HTTPMethod is actually uppercased if a lowercase value is provided.

@smartinio

smartinio commented Oct 24, 2024

Copy link
Copy Markdown
Contributor Author

You could actually improve the developer experience significantly.

Nice, but I'm just trying to fix the type error for now as it poses a runtime risk. Feel free to open a PR for the autocomplete 👍

@Uzlopak

Uzlopak commented Oct 24, 2024

Copy link
Copy Markdown
Contributor

There is no runtime risk?!

@smartinio

smartinio commented Oct 24, 2024

Copy link
Copy Markdown
Contributor Author

There is no runtime risk?!

The test case in the PR description highlights the runtime risk I'm trying to fix. If, for example you were to call req.routeOptions.method.toLowerCase() today you would have a runtime error if the method in the route is an array.

@Uzlopak

Uzlopak commented Oct 24, 2024

Copy link
Copy Markdown
Contributor

You would have a runtime risk, if you would not use typescript anyway.

@smartinio

smartinio commented Oct 24, 2024

Copy link
Copy Markdown
Contributor Author

You would have a runtime risk, if you would not use typescript anyway.

Sorry, I'm not sure I follow

mcollina
mcollina approved these changes Oct 24, 2024
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina merged commit 4ae3f67 into fastify:main Oct 24, 2024
@Uzlopak Uzlopak mentioned this pull request Oct 24, 2024
Merged
@Uzlopak

Uzlopak commented Oct 24, 2024

Copy link
Copy Markdown
Contributor

@smartinio

added a follow up pr #5766

@smartinio smartinio deleted the fix/routeOptions-method-type branch October 24, 2024 18:56
@renovate renovate Bot mentioned this pull request Jan 21, 2025
Merged
1 task
This was referenced Apr 21, 2025
Closed
Open
Closed
Open
This was referenced Apr 28, 2025
Open
Open
Open
Closed
This was referenced May 7, 2025
Open
Merged
Closed
@github-actions

github-actions Bot commented Oct 25, 2025

Copy link
Copy Markdown

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Oct 25, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mcollina mcollina mcollina approved these changes

@climba03003 climba03003 climba03003 approved these changes

Assignees

No one assigned

Labels

typescript TypeScript related

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@smartinio @Uzlopak @mcollina @climba03003