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

feat: route parameters #134

Merged
merged 7 commits into from
Apr 6, 2021
Merged

Conversation

polymathca
Copy link
Contributor

@polymathca polymathca commented Feb 23, 2021

Adds route parameters based on underscore-prefixed folders. See #133

Checklist

@polymathca
Copy link
Contributor Author

I just noticed some strange behaviour in the output of fastify.printRoutes() if a file inside the parameterized folder (e.g. _id) doesn't have a root path defined (/), but does have a named path (/test) defined.

Given

# /routes/_id/index.js

export default async function (app, opts) {
  app.get('/test', async function (req, reply) {
  })
}

Instead of showing:

├── :id/test (GET)
└── / (GET)

it displays:

├── :/testid (GET)
└── / (GET)

The application itself still functions as expected, e.g. /routes/some_id/test will respond correctly. Adding;

app.get('/', async function (req, reply) {
})

Results in:

├── :id (GET)
└── / (GET)
    └── test (GET)
        └── / (GET)

Not sure what to make of it!

@climba03003
Copy link
Member

My advice would be make use of fastify.log.debug or fastify.log.trace when you debug.
Then, just left the the log there as they are useful when you debugging. And this info should be useful when the others want to debug.

@polymathca
Copy link
Contributor Author

My advice would be make use of fastify.log.debug or fastify.log.trace when you debug.
Then, just left the the log there as they are useful when you debugging. And this info should be useful when the others want to debug.

Thanks, I'll keep that in mind! I wasn't sure where to start logging, as the route is working properly, just not displaying correctly in the printRoutes() output.

@polymathca
Copy link
Contributor Author

I added a new test to track this down. The route tree, at least on my system, looks like this:

└── / (GET)
    ├── pages (GET)
    │   └── / (GET)
    │       ├── archived (GET)
    │       └── :id (GET)
    │           └── / (GET)
    │               └── edit (GET)
    └── users/:/detailsid (GET)   <- this is scrambled, should be 'users/:id/details (GET)'

But all of the tests pass as expected, so it appears to just be an issue in how the tree is being displayed.

@mcollina
Copy link
Member

The code looks good to me.
It would be awesome if you could identify why find-my-way printRoutes scrambles it (and maybe send a PR).

I would start trying to assemble the routes manually and see if you can reproduce the rendering problem.

@polymathca
Copy link
Contributor Author

The code looks good to me.
It would be awesome if you could identify why find-my-way printRoutes scrambles it (and maybe send a PR).

I would start trying to assemble the routes manually and see if you can reproduce the rendering problem.

I believe I finally found the culprit! I created a PR here:

delvedor/find-my-way#182

(I'm not sure if github allows referencing between projects or not)

@climba03003 climba03003 linked an issue Mar 11, 2021 that may be closed by this pull request
@polymathca polymathca marked this pull request as ready for review March 31, 2021 15:23
@mcollina
Copy link
Member

mcollina commented Apr 6, 2021

The code looks ok! Could you add docs about it in the README.md?

@polymathca
Copy link
Contributor Author

I updated the documentation and added CJS tests as well, should be good to go!

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

README.md Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

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 ac6e74f into fastify:master Apr 6, 2021
mcollina pushed a commit that referenced this pull request Apr 16, 2021
* fix: add missing type routeParams

related to #134

* test: add a test for routeParams option
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 this pull request may close these issues.

Support route parameters in folder names
3 participants