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

Backport webdav methods from next to current branch #5412

Closed
2 tasks done
johaven opened this issue Apr 20, 2024 · 7 comments
Closed
2 tasks done

Backport webdav methods from next to current branch #5412

johaven opened this issue Apr 20, 2024 · 7 comments

Comments

@johaven
Copy link
Contributor

johaven commented Apr 20, 2024

Prerequisites

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

Issue

Hi team,

I would like to know if it would be possible to backport this commit (#4409) to the current branch.

It was merged a long time ago now and I'm not sure that v5 will be ready soon.

On NestJS side I have a PR waiting for these changes: nestjs/nest#13278

These changes have been expected by many for some time, with very little effort I think we could offer this feature right now.
I tested these changes and everything works fine on v4.2 (only the body parsing is missing: #5411)

@gurgunday
Copy link
Member

Moving my comment here:

Per @climba03003's comment: #4409 (review)

Backporting these shorthands could conflict with decorated properties of current users, making this breaking

@johaven
Copy link
Contributor Author

johaven commented Apr 21, 2024

Is there currently a system to prevent http methods from being used as decorators?

We could implement a way to warn the user that he will not be able to use a method name as a decorator in the next version.

In the current version we could check the name of the decorator used, display a warning and prioritize the execution of the user decorator rather than the method to avoid any break.

Just an idea :)

Even if V5 was released tomorrow, I think the NestJs team will take some time before integrating this version into their framework.
I'm just trying to find a solution to prevent the community from waiting a few more years just for some rather standard methods (which are very useful to us in our tools and applications).

@gurgunday
Copy link
Member

Is there currently a system to prevent http methods from being used as decorators?

It just throws, as an example you can try: fastify.decorate('head', () => {}), it should throw

I'm just trying to find a solution to prevent the community from waiting a few more years

I (and others) really want v5 to be released as soon as possible and we will be pushing for it, right now I just know that most maintainers are busy as node 22 (next LTS) is about to be released, so there is focus on getting as much stuff in it as possible

How about making decorated webdav methods an option (turned off by default in v4)

I would fully support that actually

@climba03003
Copy link
Member

climba03003 commented Apr 22, 2024

You are not block from using those methods by fastify.route.
It is not necessary for the shorthands method exist before using.

@mcollina
Copy link
Member

I think that creating the verb helpers could be done inside Nest if you really need them.

Generically this is not needed, because you can always use .route(). If you can't, it's on Nest.

@johaven
Copy link
Contributor Author

johaven commented Apr 22, 2024

Indeed the problem is that Nest uses the instance followed by the method to handle the requests in their adapter: https://github.com/nestjs/nest/blob/727032e46e2371a934a1a99ec42634fabd271738/packages/platform-fastify/adapters/fastify-adapter.ts#L705

@johaven
Copy link
Contributor Author

johaven commented Apr 22, 2024

I think that creating the verb helpers could be done inside Nest if you really need them.

This is probably the best solution in the meantime.

In my view, Nest's Fastify adapter should call methods from fastify.route rather than from the instance to ensure that the call is made on a method rather than a user decorator.

If there are no other comments I will close this discussion.

@Fdawgs Fdawgs closed this as not planned Won't fix, can't repro, duplicate, stale Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants