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(ngcc): compute the correct package paths for a target entry-points #40376

Conversation

petebacondarwin
Copy link
Member

Previously, if an entry-point started with the string of another package that
is included in paths mappings, then the base path was incorrectly computed
resulting in the wrong package path for the entry point.

Now we not only check whether the target path "starts with" the base path
but then also whether the target path is actually contained in the base path
from a file-system directory point of view.

Fixes #40352
Fixes #40357

@petebacondarwin petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release comp: ngcc labels Jan 9, 2021
@ngbot ngbot bot modified the milestone: Backlog Jan 9, 2021
@google-cla google-cla bot added the cla: yes label Jan 9, 2021
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.

There's also a typo in the commit message header, which mentions "a target entry-points"

Previously, if there were path-mapped entry-points, where one contaied the
string of another - for example `worker-client` and `worker` - then the
base paths were incorrectly computed resulting in the wrong package path
for the longer entry-point. This was because, when searching for a matching
base path, the strings were tested using `startsWith()`, whereas we should
only match if the path was contained in a directory from a file-system
point of view.

Now we not only check whether the target path "starts with" the base path
but then also whether the target path is actually contained in the base path
using `fs.relative()`.

Fixes angular#40352
Fixes angular#40357
@petebacondarwin petebacondarwin force-pushed the ngcc-path-containment-issue-40352 branch from 09c9c56 to 7e7769a Compare January 10, 2021 11:49
@petebacondarwin
Copy link
Member Author

Since the commit message needed updating I just amended the commit.

@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 10, 2021
@atscott atscott closed this in afd1166 Jan 11, 2021
atscott pushed a commit that referenced this pull request Jan 11, 2021
…40376)

Previously, if there were path-mapped entry-points, where one contaied the
string of another - for example `worker-client` and `worker` - then the
base paths were incorrectly computed resulting in the wrong package path
for the longer entry-point. This was because, when searching for a matching
base path, the strings were tested using `startsWith()`, whereas we should
only match if the path was contained in a directory from a file-system
point of view.

Now we not only check whether the target path "starts with" the base path
but then also whether the target path is actually contained in the base path
using `fs.relative()`.

Fixes #40352
Fixes #40357

PR Close #40376
@petebacondarwin petebacondarwin deleted the ngcc-path-containment-issue-40352 branch January 14, 2021 09:02
@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 Feb 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker 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.

Library is skipped by Ivy / ngcc compiler NGCC failure with libraries of similar names
2 participants