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

Route overridden #4114

Closed
guotingchao opened this issue Feb 20, 2020 · 6 comments · May be fixed by #4186
Closed

Route overridden #4114

guotingchao opened this issue Feb 20, 2020 · 6 comments · May be fixed by #4186
Labels
type: question 🙌

Comments

@guotingchao
Copy link

guotingchao commented Feb 20, 2020 •
edited
Loading

My problem is that the route is overwritten。

image

If I use adminApp.init() first , I can't get access to apiApp router.

I only have access to the ‘adminApp‘ module internal routes that are initialized first.
@guotingchao guotingchao added needs triage This issue has not been looked into type: question 🙌 labels Feb 20, 2020
@kamilmysliwiec kamilmysliwiec removed the needs triage This issue has not been looked into label Feb 21, 2020
@jmcdo29
Copy link
Member

jmcdo29 commented Feb 21, 2020

Here's a minimum reproduction of the issue: https://github.com/jmcdo29/nest-issue-4114

I'm not sure if this is a bug or a side effect of using multiple factories in the main.ts file. It looks like all the routes are correctly registered with Express, but one of them definitely isn't being routed to. Hopefully the reproduction helps.

@uc4w6c
Copy link
Contributor

uc4w6c commented Feb 23, 2020

I understanded the cause.

adminApp and apiApp each run registerRouterHooks methods run when init method run.
This makes the Router look like this.

  1. Router: /admin/admin
  2. Midrreware: / (NotFoundHandler)
  3. Midrreware: / (ExceptionHandler)
  4. Router: /api/api
  5. Midrreware: / (NotFoundHandler)
  6. Midrreware: / (ExceptionHandler)

by this, We can't get access to apiApp router.

Is this expected behavior?

How to respond

If so, modify express-adapter.ts as follows.

    setErrorHandler(handler, prefix) {
        return this.use('/' + prefix, handler);
    }
    setNotFoundHandler(handler, prefix) {
        return this.use('/' + prefix, handler);
    }

But this has one problem.
Json cannot be returned if the root URL is wrong as shown below.

curl http://localhost:3000/cat
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8">
<title>Error</title>
</head>
<body>
<pre>Cannot GET /cat</pre>
</body>
</html>

Please let me know if there is any other better way.
If possible, I want to start putting together a PR.

@uc4w6c
Copy link
Contributor

uc4w6c commented Feb 27, 2020 •
edited
Loading

@kamilmysliwiec
Hello! Thank you as always.
Could you tell me why did you revert the following PR in commits (fc7d94be65aacba412cd2fd7541b5628bffc2cd9) and (ea07fb7e169566b31025756a800bff3e9490626a)?
#3656

If the reason of the revert is that an error occurs when / is appended, I will fix it as follows, is that OK?

sample

public setNotFoundHandler(handler: Function, prefix?: string) {
  if (!prefix) {
    return this.use(handler);
  }
  return this.use((prefix.charAt(0) !== '/' ? '/' + prefix : prefix), handler);
}

@kamilmysliwiec
Copy link
Member

@uc4w6c it has introduced tons of breaking changes and regressions

@uc4w6c
Copy link
Contributor

uc4w6c commented Feb 28, 2020

@kamilmysliwiec
I see.

I think We can address this issue while maintaining compatibility, so I will fix it.
Please let me know if you do not want to fix it.

Thank you!

@uc4w6c uc4w6c mentioned this issue Feb 29, 2020
Open
3 tasks
@jmcdo29 jmcdo29 mentioned this issue Mar 11, 2020
Closed
@OysterD3
Copy link

OysterD3 commented Apr 19, 2020 •
edited
Loading

Still no updates for this?

EDIT: I found a workaround for this. await Promise.all([v1Api.init(), v2Api.init()])

@micalevisk micalevisk moved this to 🤔 I'll investigate later in My contributions to NestJS framework 😻 Dec 14, 2021
@github-project-automation github-project-automation bot moved this from 🤔 I'll investigate later to 😔 abandoned in My contributions to NestJS framework 😻 Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
type: question 🙌
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(platform): added prefix to adapters uc4w6c/nest
5 participants
@guotingchao @OysterD3 @kamilmysliwiec @jmcdo29 @uc4w6c