Skip to content

Fix for nested routes validation#211

Merged
cdimascio merged 1 commit intocdimascio:masterfrom
vdmitriy:master
Jan 10, 2020
Merged

Fix for nested routes validation#211
cdimascio merged 1 commit intocdimascio:masterfrom
vdmitriy:master

Conversation

@vdmitriy
Copy link
Copy Markdown

@vdmitriy vdmitriy commented Jan 9, 2020

Validator doesn't work if it installed for nested route instead root app, because when called from a middleware, the mount point is not included in req.path

Copy link
Copy Markdown
Owner

@cdimascio cdimascio left a comment

Choose a reason for hiding this comment

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

Looks good! thanks @vdmitriy

@cdimascio cdimascio merged commit f9a8aad into cdimascio:master Jan 10, 2020
@cdimascio
Copy link
Copy Markdown
Owner

@all-contributors add @vdmitriy for code and test

@allcontributors
Copy link
Copy Markdown
Contributor

@cdimascio

I've put up a pull request to add @vdmitriy! 🎉

@cdimascio
Copy link
Copy Markdown
Owner

@vdmitriy your fix is available in v3.6.1. thanks again

@cdimascio
Copy link
Copy Markdown
Owner

@vdmitriy given this new change, it appears the install methods should have their type signatures updated to indicate that either an express Application or an express Router.

is this correct?

Assuming so, would you mind updating the signatures on the 3 install methods.

For example, in src/index.ts the type signatures should indicate the acceptable params

  public async install(app: Application | Router): Promise<void>;
  public install(app: Application | Router, callback: (error: Error) => void): void;
  public install(app: Application | Router, ...

Note that making those changes will affect some of downstream call sites and functions and will require updating their types as well.

Finally, the readme should likely indicate that either app or router can be passed to the various install methods.

cdimascio added a commit that referenced this pull request Mar 2, 2025
Fix for nested routes validation
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.

2 participants