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

[FSTDEP014] FastifyDeprecation: You are trying to set/access the default route. #236

Closed
2 tasks done
evshiron opened this issue Jan 2, 2023 · 15 comments · Fixed by #238
Closed
2 tasks done

[FSTDEP014] FastifyDeprecation: You are trying to set/access the default route. #236

evshiron opened this issue Jan 2, 2023 · 15 comments · Fixed by #238

Comments

@evshiron
Copy link

evshiron commented Jan 2, 2023

Prerequisites

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

Issue

I start a fastify project with websocket today and receive the following warning:

[FSTDEP014] FastifyDeprecation: You are trying to set/access the default route. This property is deprecated. Please, use setNotFoundHandler if you want to custom a 404 handler or the wildcard (*) to match all routes.

The relevant lines of code are:

Which might require some refactoring to make it disappear.

@tobbe-r
Copy link

tobbe-r commented Jan 2, 2023

+1 Experiencing the exact same issue

@RafaelGSS
Copy link
Member

That's expected. See fastify/fastify#4480

@jsumners
Copy link
Member

jsumners commented Jan 2, 2023

Please read the notice and follow the instructions therein.

@jsumners jsumners closed this as not planned Won't fix, can't repro, duplicate, stale Jan 2, 2023
@Uzlopak
Copy link
Contributor

Uzlopak commented Jan 2, 2023

Probably open an issue in fastify-websocket to take care of the Deprecation warning and tag it with good first issue

@jsumners jsumners reopened this Jan 2, 2023
@MarianoFacundoArch
Copy link

We should fix this to use new methods?

@jsumners
Copy link
Member

jsumners commented Jan 2, 2023

We should fix this to use new methods?

Yes.

@evshiron
Copy link
Author

evshiron commented Jan 2, 2023

Thanks for the replies and just to confirm, if I get it right, because the call to deprecated methods happens in fastify/fastify-websocket (this repo), we (actually you, the maintainers) will update it someday, right?

@jsumners
Copy link
Member

jsumners commented Jan 2, 2023

Yes, this module needs to be updated. It is the community's responsibility to keep the things they use updated. This software is provided as-is.

@climba03003
Copy link
Member

Do we need to replace defaut route in this case? I see we drop hijacking error handling already, maybe just drop default route in this case.
cc @airhorns

@mcollina
Copy link
Member

mcollina commented Jan 3, 2023

I would need to see what tests breaks. Ideally, that's what we would do.

@xtremespb
Copy link

What about replacing the default route logic by prehandler?

fastify.addHook("preHandler", (request, reply, done) => {
    if (request[kWs]) {
        handleUpgrade(request, (connection) => {
            noHandle.call(fastify, connection, request);
        });
    }
    done();
});

@climba03003
Copy link
Member

climba03003 commented Jan 3, 2023

What about replacing the default route logic by prehandler?

No, they are not identical. pre-handler will not run for the route that is not exist.
And, we will be upgrading twice if we add a pre-handler.

Removal of default route means when the route is not explicitly defined. No upgrade will be allowed.
Whereas, currently it upgraded and close afterward.

@mcollina
Copy link
Member

mcollina commented Jan 3, 2023

Would that change the behavior significantly?

@climba03003
Copy link
Member

Would that change the behavior significantly?

I believe the impact would be minimal.

Current

sequenceDiagram
    Broswer->>+Server: Initial Connection with WebScoket
    Server->>-Broswer: 101 Switch Protocol
    Broswer->>+Server: Connection Establish
    Server->>-Broswer: Immediate Close
    Broswer->>+Broswer: Handle by .onclose

After removal of default route

sequenceDiagram
    Broswer->>+Server: Initial Connection with WebScoket
    Server->>-Broswer: 404 Not Found
    Broswer->>+Broswer: Handle by .onerror or .onclose

@mcollina
Copy link
Member

mcollina commented Jan 3, 2023

Seems even better. My guess is that was a relic of some past code.

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

Successfully merging a pull request may close this issue.

9 participants