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

should throw for non-string route URL #3648

Closed
2 tasks done
Eomm opened this issue Jan 23, 2022 · 2 comments · Fixed by #3653
Closed
2 tasks done

should throw for non-string route URL #3648

Eomm opened this issue Jan 23, 2022 · 2 comments · Fixed by #3653
Labels
good first issue Good for newcomers semver-minor Issue or PR that should land as semver minor

Comments

@Eomm
Copy link
Member

Eomm commented Jan 23, 2022

Prerequisites

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

Issue

This snippet starts correctly:

const fastify = require('fastify')({ logger: true })

const handler = async (request, reply) => {
  return { hello: request.params.foo }
}

fastify.get(/^\/(donations|skills|blogs)/, handler)
fastify.listen(8080)

There is one error: the regExp on the route's URL.
In this case, I think we should throw an error.

Note that find-my-way is receiving a string because we are composing its route input checking the prefix etc..

Do you agree?

@mcollina
Copy link
Member

Definitely!

@mcollina mcollina added good first issue Good for newcomers enhancement v4.x Issue or pr related to Fastify v4 semver-major Issue or PR that should land as semver major labels Jan 23, 2022
@jsumners
Copy link
Member

I think it's a semver-patch. It currently does not work, but does so without any warning.

@mcollina mcollina removed semver-major Issue or PR that should land as semver major v4.x Issue or pr related to Fastify v4 labels Jan 23, 2022
VigneshMurugan added a commit to VigneshMurugan/fastify that referenced this issue Jan 24, 2022
VigneshMurugan added a commit to VigneshMurugan/fastify that referenced this issue Jan 24, 2022
VigneshMurugan added a commit to VigneshMurugan/fastify that referenced this issue Jan 24, 2022
@climba03003 climba03003 linked a pull request Jan 24, 2022 that will close this issue
4 tasks
mcollina pushed a commit that referenced this issue Jan 25, 2022
* Fixed #3648 - URL must be a string

* Revert "Fixed #3648 - URL must be a string"

This reverts commit c5cd59f.

* Fixed #3648 - URL must be a string

* Fixed GitHub Linter Error

* Fixed GitHub Linter Error

* Fixed GitHub Linter Error

* Fixed GitHub Linter Error

* Update lib/route.js

Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>

* Update route.test.js

* Addressed Review comments - Used Error codes. Removed unecessary url existence check

* Removed unecessary logger statement

Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>
@Eomm Eomm added the semver-minor Issue or PR that should land as semver minor label Apr 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers semver-minor Issue or PR that should land as semver minor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants