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): capture entry-point dependencies from typings as well as source #34494

Conversation

@petebacondarwin
Copy link
Member

petebacondarwin commented Dec 19, 2019

See FW-1781 for the background

Fixes #34411

@JoostK

This comment has been minimized.

Copy link
Member

JoostK commented Dec 19, 2019

Could you update the commit message to include the fixes #34411 reference?

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngcc-typings-dependency-host branch 2 times, most recently from 8675f98 to 1bdb64e Dec 19, 2019
@petebacondarwin petebacondarwin requested a review from JoostK Dec 19, 2019
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngcc-typings-dependency-host branch from 1bdb64e to 75dadbf Dec 20, 2019
Copy link
Member

gkalpak left a comment

OOC, have you done any measurements regarding how much in the overall performance affected by this change?

Rather than return a new object of dependency info from calls to
`collectDependencies()` we now pass in an object that will be updated
with the dependency info. This is in preparation of a change where
we will collect dependency information from more than one
`DependencyHost`.

Also to better fit with this approach the name is changed from
`findDependencies()` to `collectDependencies()`.
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngcc-typings-dependency-host branch from 75dadbf to 4ca6faf Dec 23, 2019
The `DependencyHost` implementations were duplicating the "postfix" strings
which are used to find matching paths when resolving module specifiers.
Now the hosts reuse the postfixes given to the `ModuleResolver` that is
passed to the host.
…ource

ngcc computes a dependency graph of entry-points to ensure that
entry-points are processed in the correct order. Previously only the imports
in source files were analysed to determine the dependencies for each
entry-point.

This is not sufficient when an entry-point has a "type-only" dependency
 - for example only importing an interface from another entry-point.
In this case the "type-only" import does not appear in the
source code. It only appears in the typings files. This can cause a
dependency to be missed on the entry-point.

This commit fixes this by additionally processing the imports in the
typings program, as well as the source program.

Note that these missing dependencies could cause unexpected flakes when
running ngcc in async mode on multiple processes due to the way that
ngcc caches files when they are first read from disk.

Fixes #34411

// FW-1781
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngcc-typings-dependency-host branch from 4ca6faf to d21b34e Dec 23, 2019
@petebacondarwin

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Dec 23, 2019

RE performance change: note that this change only affects the dependency analysis phase, which is around 7.4% of the total processing time (using integration/cli-hello-world-ivy-compat). After this PR the average percentage goes up to around 7.8%.

@alxhub
alxhub approved these changes Jan 7, 2020
alxhub added a commit that referenced this pull request Jan 7, 2020
Rather than return a new object of dependency info from calls to
`collectDependencies()` we now pass in an object that will be updated
with the dependency info. This is in preparation of a change where
we will collect dependency information from more than one
`DependencyHost`.

Also to better fit with this approach the name is changed from
`findDependencies()` to `collectDependencies()`.

PR Close #34494
alxhub added a commit that referenced this pull request Jan 7, 2020
#34494)

The `DependencyHost` implementations were duplicating the "postfix" strings
which are used to find matching paths when resolving module specifiers.
Now the hosts reuse the postfixes given to the `ModuleResolver` that is
passed to the host.

PR Close #34494
alxhub added a commit that referenced this pull request Jan 7, 2020
…ource (#34494)

ngcc computes a dependency graph of entry-points to ensure that
entry-points are processed in the correct order. Previously only the imports
in source files were analysed to determine the dependencies for each
entry-point.

This is not sufficient when an entry-point has a "type-only" dependency
 - for example only importing an interface from another entry-point.
In this case the "type-only" import does not appear in the
source code. It only appears in the typings files. This can cause a
dependency to be missed on the entry-point.

This commit fixes this by additionally processing the imports in the
typings program, as well as the source program.

Note that these missing dependencies could cause unexpected flakes when
running ngcc in async mode on multiple processes due to the way that
ngcc caches files when they are first read from disk.

Fixes #34411

// FW-1781

PR Close #34494
@alxhub alxhub closed this in e2b1845 Jan 7, 2020
alxhub added a commit that referenced this pull request Jan 7, 2020
#34494)

The `DependencyHost` implementations were duplicating the "postfix" strings
which are used to find matching paths when resolving module specifiers.
Now the hosts reuse the postfixes given to the `ModuleResolver` that is
passed to the host.

PR Close #34494
alxhub added a commit that referenced this pull request Jan 7, 2020
…ource (#34494)

ngcc computes a dependency graph of entry-points to ensure that
entry-points are processed in the correct order. Previously only the imports
in source files were analysed to determine the dependencies for each
entry-point.

This is not sufficient when an entry-point has a "type-only" dependency
 - for example only importing an interface from another entry-point.
In this case the "type-only" import does not appear in the
source code. It only appears in the typings files. This can cause a
dependency to be missed on the entry-point.

This commit fixes this by additionally processing the imports in the
typings program, as well as the source program.

Note that these missing dependencies could cause unexpected flakes when
running ngcc in async mode on multiple processes due to the way that
ngcc caches files when they are first read from disk.

Fixes #34411

// FW-1781

PR Close #34494
@petebacondarwin petebacondarwin deleted the petebacondarwin:ngcc-typings-dependency-host branch Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.