The nest build Command may Resolve Module Paths Incorrectly

[astormnewrelic]

I'm submitting a...

[ ] Regression 
[x] Bug report
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

We've had some of our users report problems getting New Relic's Node.js Agent working with a NestJS project. We think we've tracked the problem down to what looks like a bug in lib/compiler/hooks/tsconfig-paths.hook.ts. We'd like to get this fixed, but we're not 100% sure what the intended behavior of this hook/plugin is and unsure how to go about a PR safely.

Current behavior

When running nest build/npm run build, if

1. A user's typescript code imports a CommonJS module from node_modules
2. And there's a javascript file with the same name as the module name in the project root

then the compiled output of the program will contain a require statement with an incorrect path. This creates a problem for users of the Node.js agent, as our configuration can be stored in the root of a project in a file named newrelic.js (and our NPM module name is also newrelic).

Expected behavior

When resolving module paths, nest build should prefer modules installed in node_modules to a javascript file in the root of a project.

Minimal reproduction of the problem with instructions

See also: https://github.com/astormnewrelic/nestjs-compiler-repro

1. Run $ nest new project-name (choosing an NPM managed project)
2. Run $ cd project-name
3. Run $ npm install newrelic
4. Run $ touch newrelic.js
5. Modify src/main.js so it matches the program below
6. Run $ npm run build
7. Examine ./dist/main.js

Expected Behavior:

Compiled output contains: const newrelic_1 = require("newrelic");

Actual Behavior:

Compiled output contains: const newrelic_1 = require("../newrelic");

Modified Program: (mentioned in step #5)

// importing newrelic
import newrelic from 'newrelic';

// using the newrelic object so it doesn't' get optimized out by the compiler
console.log(newrelic)
import { NestFactory } from '@nestjs/core';
import { AppModule } from './app.module';

async function bootstrap() {
  const app = await NestFactory.create(AppModule);
  await app.listen(3000);
}
bootstrap();

What is the motivation / use case for changing the behavior?

We'd like users of the New Relic Node.js agent to be able to more easily use the Agent with a Nest application. This is not a theoretical problem -- we have users report this problem to us.

We'd also like the Nest compilation process to match the behavior of a stock typescript application as much as possible.

Environment

| \ | |           | |    |_  |/  ___|/  __ \| |   |_   _|
|  \| |  ___  ___ | |_     | |\ `--. | /  \/| |     | |
| . ` | / _ \/ __|| __|    | | `--. \| |    | |     | |
| |\  ||  __/\__ \| |_ /\__/ //\__/ /| \__/\| |_____| |_
\_| \_/ \___||___/ \__|\____/ \____/  \____/\_____/\___/


[System Information]
OS Version     : macOS Mojave
NodeJS Version : v12.16.1
NPM Version    : 6.13.4 

[Nest CLI]
Nest CLI Version : 7.4.1 

[Nest Platform Information]
platform-express version : 7.0.0
common version           : 7.0.0
core version             : 7.0.0

[astormnewrelic]

Repro repository created at https://github.com/astormnewrelic/nestjs-compiler-repro

[jmcdo29]

@kamilmysliwiec I'm gonna need some input here: I found that the issue comes from the tsconfig-paths's createMatchPath function and using ./ as thebaseUrl in thetsconfig.json instead of ./src. Should this be fixed at the Nest compiler level, or should this be fixed in the tsconfig.json?

[kamilmysliwiec]

Thanks for reporting @astormnewrelic. @jmcdo29 if there's a way we could fix it at the compiler level, that would be great 👍 would you like to dive into this?

[jmcdo29]

As anyone using the nest build command should be using a nest-cli.json, would we be able to read the sourceRoot property from it? I'm trying to think of a way we can ensure we're reading from the proper source directory without hard-coding the directory so it can still work for monorepos and for standard repos. Do you have any thoughts @kamilmysliwiec?

[kamilmysliwiec]

We can try to read it and - in case it doesn't exist/cannot be found - catch the error and fallback to the default value

[kamilmysliwiec]

I'm not 100% sure but this property might be available already (somewhere in the compiler's code). I would have to double-check though @jmcdo29 😅

[jmcdo29]

Guess I'll start digging. The other option I'm thinking of is updating the typescript-starter repository to have the tsconfig.build.json to have the baseUrl property set to be src so that the module resolution is scoped only to the src directory, but I could see this being a problem if people, for whatever reason, decide to have a build structure like

root
--config
--src

[kamilmysliwiec]

It was set to be src in the past and we had to change it due to numerous issues people encountered (one of them was the build structure that you've shown above)

[jmcdo29]

I've been thinking on this for a few days now. How do we handle a directory structure like above? If we look in sourceRoot (src) then we miss the root level config directory that people may be using. If we look in the project root (as we already are) we could reference a file instead of a package on accident. We could see if the file name is a package name as well, but that may introduce more complexities and breaking cases. Do we make the breaking change and say that there should be practically no situation where there's typescript outside of src (and if there is it's up to the developer to know what they're doing and why), or should this fix come from a different place?

[astormnewrelic]

@jmcdo29 @kamilmysliwiec Is there anything we can do to help triage this and move along to a solution? It sounds like you're caught in a "doing the right thing means breaking backward compatibility trap" -- which I can sympathize with. I know it's easy for me to say this but this might be a situation where it's best to bite the bullet and change the behavior to only scan src for TypeScript.

If you can't apply a generic fix, would you be open to a one off special case for the newrelic.js file? I know that's a little gross pragmatic, but from our perspective we want folks to be able to install NestJS, install New Relic, and move on without any additional configuration, and having this file special cased would achieve that.

[jmcdo29]

@astormnewrelic The problem lies in how tsconfig-paths searches for renamed modules, to allow for path aliasing, which is nice to not have imports like ../../../../../../interfaces or some other ugly kind of path (gross exaggeration (I hope), but it gets the point across). Normally tsconfig-paths looks based on the baseUrl property of the tsconfig that is used to build the project. This is great when good typescript practices are followed (all source code in one directory), but can lead to problems when you have a directory structure as mentioned here.

@kamilmysliwiec I'm in support of the breaking change back to setting the baseUrl as src. Nest is an opinionated framework, and it's at least my opinion that this kind of structure is a bad practice. Maybe we could make a mention in the docs that source code should be in a single directory? Monorepos already follow this with all of the source code under apps/, I feel that enforcing that with src/ would be good as well. And if someone wanted to change it, they'd be welcome to modify their tsconfig file with baseUrl: './' and accept the consequences that may come with the configuration. Similar to how developers can decide to not follow Nest's directory structure for files, this would be another choice they could make to do something different than Nest's opinion.

[kamilmysliwiec]

I can't agree on setting the baseUrl as src as this led to many errors in the past (you can find more details in the nestjs/nest closed issues). I've seen setups in which some files (like migrations or configs, or just scripts) are not located in the src folder and I wouldn't necessarily call it a bad practice (although it's not how I structure things).

I still believe there's a way to fix this issue without introducing other ones. I will look into this asap.

[kamilmysliwiec]

Fixed in 7.5.2

[Farenheith]

Hello!

It looks like some similar problem has been introduced in @nestjs/cli@10.
I have a file with the following import:

import { GenerateCommand } from 'lib/application/command';

I also have the following path configured in my tsconfig.json:

      "lib/*": [
        "./lib/*"
      ],

After I compile (commonJs) the application with cli version 10.0.5, I get this error when trying to run it:

ReferenceError: command_1 is not defined

I checked and found out that it happened because the require equivalent to the /command import is just not present in the generated js.

I downgraded to @nestjs/cli@9.5.0 and I tried to compile and run it again, and everything went fine.

[micalevisk]

@Farenheith

[Farenheith]

@Farenheith

I'm just commenting here because I can't open a proper issue right now, but as soon as I can I'll create a repo to simulate this problem to do so. I know it's closed 3 years ago.

Edit: Just to make it clearer, @micalevisk. Commenting here is important because someone with the same problem that cares to google it can now find a solution to it until it is fixed. You're welcome.