nest icon indicating copy to clipboard operation
nest copied to clipboard

Route overridden

Open guotingchao opened this issue 4 years ago • 6 comments

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 avatar Feb 20 '20 12:02 guotingchao

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.

jmcdo29 avatar Feb 21 '20 17:02 jmcdo29

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 avatar Feb 23 '20 10:02 uc4w6c

@kamilmysliwiec Hello! Thank you as always. Could you tell me why did you revert the following PR in commits (fc7d94be65aacba412cd2fd7541b5628bffc2cd9) and (ea07fb7e169566b31025756a800bff3e9490626a)? https://github.com/nestjs/nest/pull/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);
}

uc4w6c avatar Feb 27 '20 15:02 uc4w6c

@uc4w6c it has introduced tons of breaking changes and regressions

kamilmysliwiec avatar Feb 28 '20 07:02 kamilmysliwiec

@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 avatar Feb 28 '20 08:02 uc4w6c

Still no updates for this?

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

OysterD3 avatar Apr 19 '20 15:04 OysterD3