nestjs / nest

A progressive Node.js framework for building efficient, scalable, and enterprise-grade server-side applications with TypeScript/JavaScript 🚀
https://nestjs.com
MIT License
67.7k stars 7.63k forks source link

[Bug] @nodejs/common Exceptions thrown in an imported "npm linked" NestJS Module will give 500 instead of expected non-200 error code. #4674

Closed mdaum closed 4 years ago

mdaum commented 4 years ago

I have created a repo that reproduces the issue: https://github.com/mdaum/NestJSNpmLinkBug

In it are two NestJS modules, Module A and Module B. Module A exposes GET route say/:key, which will print out the value of the key parameter, unless it is badWord, in which case it will respond with a 400. Module B is a simple starter app that imports Module A.

It can be observed that when this is done with a direct import (i.e import {ModuleA} from '../../moduleA/src/app.module';) we see the desired behavior.

However, when imported via npm link (i.e import {ModuleA} from 'module_a'; after linking) we will see a 500 instead of a 400 with the following logged in the terminal:

[Nest] 4136   - 04/25/2020, 1:55:46 AM   [NestFactory] Starting Nest application...
[Nest] 4136   - 04/25/2020, 1:55:46 AM   [InstanceLoader] ModuleA dependencies initialized +17ms
[Nest] 4136   - 04/25/2020, 1:55:46 AM   [InstanceLoader] AppModule dependencies initialized +1ms
[Nest] 4136   - 04/25/2020, 1:55:46 AM   [RoutesResolver] AppController {}: +4ms
[Nest] 4136   - 04/25/2020, 1:55:46 AM   [RouterExplorer] Mapped {, GET} route +3ms
[Nest] 4136   - 04/25/2020, 1:55:46 AM   [RoutesResolver] ModuleAController {}: +0ms
[Nest] 4136   - 04/25/2020, 1:55:46 AM   [RouterExplorer] Mapped {/say/:key, GET} route +2ms
[Nest] 4136   - 04/25/2020, 1:55:46 AM   [NestApplication] Nest application successfully started +3ms
[Nest] 4136   - 04/25/2020, 1:56:00 AM   [ExceptionsHandler] do not know bad word +13581ms
Error: do not know bad word
    at ModuleAController.saySomething (/home/mdaum/NestJSNpmLinkBug/moduleA/dist/app.controller.js:19:19)
    at /home/mdaum/NestJSNpmLinkBug/moduleB/node_modules/@nestjs/core/router/router-execution-context.js:37:29
    at process._tickCallback (internal/process/next_tick.js:68:7)

It would seem that NestJS is not seeing the @nestjs/common supplied exceptions as valid when being thrown in a linked module.

mdaum commented 4 years ago

NestJS Version 7.0.9

kamilmysliwiec commented 4 years ago

This is not a bug. Please, use our Discord channel (support) for such questions. We are using GitHub to track bugs, feature requests, and potential improvements.

areese159 commented 4 years ago

While I agree and understand wanting to have people ask questions in Discord before posting in Github, that doesn't change the fact that the question has already been asked. Instead of just saying "This is not a bug", why not explain it so other people who come across this issue understand what's going on. While discord is an amazing platform for helping people quickly, its search is not as powerful as Google. Most people are going to find this question on Github and not know what to do. Instead of having multiple people continue to ask the same question in Discord why don't we attempt to answer it here once. That way you can point people to the issue on Github from Discord and people on Google can find their answer easily.

areese159 commented 4 years ago

@kamilmysliwiec Please correct if I am wrong, but I am guessing the issue at hand is how @mdaum is handling @nestjs/common dependency. When nest uses common in its repo it uses it as a peer dependency, this tells the consumer that they must provide the @nestjs/common dependency. It does this because nestjs and other packages want to do comparisons to check if instances of the object are equal. When you do a comparison of object instanceof @nestjs/common with a peer dependency it returns true since they are indeed the same dependency. Remember we supplied the dependency to the package that requested it as a peer, so they really are the same thing. In your repo when you do npm link, you are not sharing the same @nestjs/common anymore. So when nestjs does the instanceof check that comparison fails and nestjs converts it to a 500 error.

salamancajr commented 3 years ago

did you ever find a solution? If so, can you provide it? @areese159 @mdaum. Per areese159's comment, I installed @nestjs/common in the linked dependency but had no luck

mdaum commented 3 years ago

Tldr: no. This really only posed a problem when testing out some nestjs providers we expose in a node module on a dev's machine. Since it was only happening when trying to debug code locally, it wasn't really important to us to find a workaround. If I understand @areese159's explanation, simply installing @nestjs/common in your linked dependency (assuming that is done via devDependency) doesn't fix the issue, rather it is the reason for the issue itself. This is because the linked dependency and the project you are attempting to use it in are using two different @nestjs/common installation locations. Therefore Nest's class equivalency check on the error thrown in your nest application will not succeed and your non-200 error will always result in a 500. However, it would work during normal use (using the published node module, not linking local version), thus giving you the non-200 status codes you expect. Just look out for this behavior when testing out a module via npm link, and take it with a grain of salt, your implementation of your node module may very well be fine.

jmcdo29 commented 3 years ago

@mdaum absolutely correct! You nailed it. Kind of one of the pain points of peer Deps and class checks in Typescript.

areese commented 3 years ago

lol, as much as I'd like to take credit you mean @areese159