Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(compiler-cli): resolve rootDir to absolute #36291

Closed
wants to merge 1 commit into from

Conversation

Toxicable
Copy link

PR Checklist

PR Type

  • Feature

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?

  • No

@pullapprove pullapprove bot requested a review from kara March 28, 2020 05:10
@Toxicable
Copy link
Author

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 alan-agius4 requested a review from alxhub April 8, 2020 08:21
@alan-agius4 alan-agius4 added the area: compiler Issues related to `ngc`, Angular's template compiler label Apr 8, 2020
@ngbot ngbot bot added this to the needsTriage milestone Apr 8, 2020
@alan-agius4 alan-agius4 removed the request for review from kara April 8, 2020 08:22
@alan-agius4 alan-agius4 added target: patch This PR is targeted for the next patch release action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 8, 2020
Copy link
Member

@JoostK JoostK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to only replace absoluteFrom with resolve, that resolves (sorry for the pun) the issue for me as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually you could replace the whole return statement with:

return rootDirs.map(rootDir => fs.resolve(cwd, rootDir);

@jbedard
Copy link
Contributor

jbedard commented Oct 17, 2020

@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...

@Toxicable
Copy link
Author

@jbedard Ah yeah if you wanna take this one then that's cool with me

@jbedard
Copy link
Contributor

jbedard commented Oct 18, 2020

@JoostK is there an example of a test similar to what you're suggesting with NgtscTestEnvironment? Would this be testing a full run of ngtsc as opposed to testing only readConfiguration or readCommandLineAndConfiguration?

If we were to do something like convertToOptionsWithAbsolutePaths in typescript do you think that would go somewhere such as readConfiguration?

@petebacondarwin
Copy link
Member

@jbedard - you could certainly create a simply unit test in a new file typescript_spec.ts at https://github.com/angular/angular/tree/master/packages/compiler-cli/src/ngtsc/util/test.

I would run this test using runInEachFileSystem() to provide a mock FileSystem for the tests.

I am not convinced that an e2e test is particularly needed.

@petebacondarwin petebacondarwin added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 10, 2020
literalpie added a commit to literalpie/angular that referenced this pull request Mar 28, 2021
Building off of angular#36291 with test added and other recomended changes.

Closes angular#36290
literalpie added a commit to literalpie/angular that referenced this pull request Mar 28, 2021
Building off of angular#36291 with test added and other recommended changes.

Closes angular#36290
@literalpie
Copy link
Contributor

I'm taking a stab at this in #41359

literalpie added a commit to literalpie/angular that referenced this pull request Mar 28, 2021
Building off of angular#36291 with test added and other recommended changes.

Closes angular#36290
literalpie added a commit to literalpie/angular that referenced this pull request Mar 28, 2021
building off of angular#36291 with test added and other recommended changes
Closes angular#36290
literalpie added a commit to literalpie/angular that referenced this pull request Mar 28, 2021
building off of angular#36291 with test added and other recommended changes. Closes angular#36290
@petebacondarwin
Copy link
Member

Closing in favour of #41359

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews area: compiler Issues related to `ngc`, Angular's template compiler cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants