fix(compiler-cli): resolve rootDir to absolute#36291
fix(compiler-cli): resolve rootDir to absolute#36291Toxicable wants to merge 1 commit intoangular:masterfrom
Conversation
|
Im not 100% certain this is the place to make this change so would appciate some feedback on that. For context I'd like to land this so that we can remove the patch found here: https://github.com/bazelbuild/rules_nodejs/pull/1619/files#diff-5eb8729ae1c44d09b4ddc25c90585b9a |
alan-agius4
added
the
area: compiler
alan-agius4
added
target: patch
There was a problem hiding this comment.
I was curious how this is handled in tsc, and they preprocess the command line args using the internal ts.convertToOptionsWithAbsolutePaths. Maybe we can implement a similar approach, to achieve consistent handling of compiler options.
Regardless, this change would need a test. You can probably extend NgtscTestEnvironment with a method to configure additional command line args, and then pass those through in driveMain and driveDiagnostics.
| return rootDirs.map(rootDir => absoluteFrom(rootDir)); | ||
| return rootDirs.map(rootDir => { | ||
| const rooted = fs.isRooted(rootDir) ? rootDir : fs.join(cwd, rootDir); | ||
| return absoluteFrom(rooted); |
There was a problem hiding this comment.
You should be able to only replace absoluteFrom with resolve, that resolves (sorry for the pun) the issue for me as well.
There was a problem hiding this comment.
Actually you could replace the whole return statement with:
return rootDirs.map(rootDir => fs.resolve(cwd, rootDir);
|
@Toxicable any update on this? I tried patching this along with the recommendation from @JoostK and it seems to be working. I could take it over if you'd like... |
|
@jbedard Ah yeah if you wanna take this one then that's cool with me |
|
@JoostK is there an example of a test similar to what you're suggesting with If we were to do something like |
|
@jbedard - you could certainly create a simply unit test in a new file I would run this test using I am not convinced that an e2e test is particularly needed. |
petebacondarwin
added
action: cleanup
|
I'm taking a stab at this in #41359 |
|
Closing in favour of #41359 |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
7 participants
PR Checklist
PR Type
What is the current behavior?
Issue Number: #36290
What is the new behavior?
Allows for a relative rootDir to be passed, usually via the command line which we will join to the cwd
Does this PR introduce a breaking change?