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.

Sign up for GitHub

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

Jump to bottom

Use fmw.hasRoute for fastify.hasRoute #5092

Closed
2 tasks done
ivan-tymoshenko opened this issue Oct 12, 2023 · 7 comments
Closed
2 tasks done

Use fmw.hasRoute for fastify.hasRoute #5092

ivan-tymoshenko opened this issue Oct 12, 2023 · 7 comments
Labels
good first issue Good for newcomers semver-major Issue or PR that should land as semver major v5.x Issue or pr related to Fastify v5
Milestone
v5.0.0

Comments

@ivan-tymoshenko
Copy link
Member

ivan-tymoshenko commented Oct 12, 2023 •
edited

Prerequisites

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

Issue

If we do this, this will be a breaking change so it should be done in the next major.
Having that this is a breaking change, it might be also a reasonable solution to keep it as it, but IMHO fastify.hasRoute doesn't work correctly/doesn't make much sense in current implementation.
@ivan-tymoshenko ivan-tymoshenko changed the title Replace use fmw.hasRoute for fastify.hasRoute Use fmw.hasRoute for fastify.hasRoute Oct 12, 2023
@Uzlopak
Copy link
Contributor

Uzlopak commented Oct 12, 2023

If hasRoute is bugged, cant we do it in a semver-minor?

@ivan-tymoshenko
Copy link
Member Author

I'm ok with both minor and major. IMHO it's more safe to ship a breaking change with a major, especially if it's not a big problem (at least I don't see any issues about that).

@mcollina
Copy link
Member

Why this issue is in fast-json-stringify?

@ivan-tymoshenko
Copy link
Member Author

Why this issue is in fast-json-stringify?

my bad. can you transfer to fastify repo pls?

@jsumners jsumners transferred this issue from fastify/fast-json-stringify Oct 12, 2023
@metcoder95 metcoder95 added semver-major Issue or PR that should land as semver major v5.x Issue or pr related to Fastify v5 labels Oct 12, 2023
@mcollina
Copy link
Member

In what sense hasRoute does not work?

@ivan-tymoshenko
Copy link
Member Author

In what sense hasRoute does not work?

  1. fastify.hasRoute searches route using a fmw.find method.
  2. fmw.find designed to find a route by url received from a client.
  3. fastify.hasRoute IMHO should search for route using a path that was used to set the route.

@mcollina mcollina added this to the v5.0.0 milestone Oct 16, 2023
@mcollina mcollina added the good first issue Good for newcomers label Oct 16, 2023
@dancastillo dancastillo mentioned this issue Oct 17, 2023
Merged
4 tasks
@Uzlopak
Copy link
Contributor

Uzlopak commented Jan 26, 2024

@dancastillo

This landed in the next branch, right? So can we close this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Good for newcomers semver-major Issue or PR that should land as semver major v5.x Issue or pr related to Fastify v5
Projects
None yet
Milestone
v5.0.0
Development

No branches or pull requests
4 participants
@mcollina @Uzlopak @metcoder95 @ivan-tymoshenko