Closed astormnewrelic closed 3 years ago
Repro repository created at https://github.com/astormnewrelic/nestjs-compiler-repro
@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
?
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?
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?
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
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 ๐
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
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)
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?
@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.
@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.
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.
Fixed in 7.5.2
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.
@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.
I'm submitting a...
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
, ifimports
a CommonJS module fromnode_modules
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 namednewrelic.js
(and our NPM module name is alsonewrelic
).Expected behavior
When resolving module paths,
nest build
should prefer modules installed innode_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
$ nest new project-name
(choosing an NPM managed project)$ cd project-name
$ npm install newrelic
$ touch newrelic.js
src/main.js
so it matches the program below$ npm run build
./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)
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