Skip to content

ngcc - handle multiple basePaths & use typings for dependency resolution#29643

Closed
petebacondarwin wants to merge 13 commits into
angular:masterfrom
petebacondarwin:FW-1204
Closed

ngcc - handle multiple basePaths & use typings for dependency resolution#29643
petebacondarwin wants to merge 13 commits into
angular:masterfrom
petebacondarwin:FW-1204

Conversation

@petebacondarwin
Copy link
Copy Markdown
Contributor

@petebacondarwin petebacondarwin commented Apr 1, 2019

Resolves FW-1204

@petebacondarwin petebacondarwin added state: WIP refactoring Issue that involves refactoring or code-cleanup area: core Issues related to the framework runtime target: major This PR is targeted for the next major release comp: ivy labels Apr 1, 2019
@ngbot ngbot Bot added this to the needsTriage milestone Apr 1, 2019
@petebacondarwin petebacondarwin force-pushed the FW-1204 branch 4 times, most recently from 60e6757 to e764fa1 Compare April 3, 2019 20:12
@petebacondarwin petebacondarwin changed the title Fw 1204 ngcc - handle multiple basePaths & use typings for dependency resolution Apr 4, 2019
@petebacondarwin petebacondarwin added effort2: days freq2: medium action: review The PR is still awaiting reviews from at least one requested reviewer risk: medium severity2: inconvenient type: bug/fix feature Label used to distinguish feature request from other issues workaround2: non-obvious and removed state: WIP labels Apr 4, 2019
@ngbot ngbot Bot modified the milestones: needsTriage, Backlog Apr 4, 2019
@petebacondarwin petebacondarwin marked this pull request as ready for review April 4, 2019 20:54
@petebacondarwin petebacondarwin requested review from a team April 4, 2019 20:54
Comment thread packages/compiler-cli/ngcc/src/packages/entry_point_finder.ts Outdated
…ters

The `Transformer` and `Renderer` classes do not
actually need a `sourcePath` value as by the time
they are doing their work we are only working directly
with full absolute paths.
This method was poorly named for what it does, and did not have a
return type.
The test now attempts to compile an entry-point (@angular/common/http/testing)
that has a transient "private" dependency. A private dependency is one that is
only visible by looking at the compiled JS code, rather than the generated TS
typings files.

This proves that we can't rely on typings files alone for computing the
dependencies between entry-points.
Previously we completely ignored entry-points that had not been
compiled with Angular, since we do not need to compile them
with ngcc. But this makes it difficult to reason about dependencies
between entry-points that were compiled with Angular and those that
were not.

Now we do track these non-Angular compiled entry-points but they
are marked as `compiledByAngular: false`.
When working out the dependencies between entry-points
ngcc must parse the import statements and then resolve the
import path to the actual file.  This is complicated because module
resolution is not trivial.

Previously ngcc used the node.js `require.resolve`, with some
hacking to resolve modules. This change refactors the `DependencyHost`
to use a new custom `ModuleResolver`, which is optimized for this use
case.

Moreover, because we are in full control of the resolution,
we can support TS `paths` aliases, where not all imports come from
`node_modules`. This is the case in some CLI projects where there are
compiled libraries that are stored locally in a `dist` folder.
See //FW-1210.
By passing a `pathMappings` configuration (a subset of the
`ts.CompilerOptions` interface), we can instuct ngcc to process
additional paths outside the `node_modules` folder.
For UMD/RequireJS support we will need to have multiple
`DependencyHost` implementations. This commit  prepares the
ground for that.
This commit introduces a new interface, which abstracts access
to the underlying `FileSystem`. There is initially one concrete
implementation, `NodeJsFileSystem`, which is simply wrapping the
`fs` library of NodeJs.

Going forward, we can provide a `MockFileSystem` for test, which
should allow us to stop using `mock-fs` for most of the unit tests.
We could also implement a `CachedFileSystem` that may improve the
performance of ngcc.
…al-path`

The ngcc code now uses `AbsoluteFsPath` and `PathSegment` to do all its
path manipulation.
@petebacondarwin
Copy link
Copy Markdown
Contributor Author

Just merging into master now that RC has been released.

AndrewKushnir pushed a commit that referenced this pull request Apr 29, 2019
)

This method was poorly named for what it does, and did not have a
return type.

PR Close #29643
AndrewKushnir pushed a commit that referenced this pull request Apr 29, 2019
The test now attempts to compile an entry-point (@angular/common/http/testing)
that has a transient "private" dependency. A private dependency is one that is
only visible by looking at the compiled JS code, rather than the generated TS
typings files.

This proves that we can't rely on typings files alone for computing the
dependencies between entry-points.

PR Close #29643
AndrewKushnir pushed a commit that referenced this pull request Apr 29, 2019
Previously we completely ignored entry-points that had not been
compiled with Angular, since we do not need to compile them
with ngcc. But this makes it difficult to reason about dependencies
between entry-points that were compiled with Angular and those that
were not.

Now we do track these non-Angular compiled entry-points but they
are marked as `compiledByAngular: false`.

PR Close #29643
AndrewKushnir pushed a commit that referenced this pull request Apr 29, 2019
AndrewKushnir pushed a commit that referenced this pull request Apr 29, 2019
When working out the dependencies between entry-points
ngcc must parse the import statements and then resolve the
import path to the actual file.  This is complicated because module
resolution is not trivial.

Previously ngcc used the node.js `require.resolve`, with some
hacking to resolve modules. This change refactors the `DependencyHost`
to use a new custom `ModuleResolver`, which is optimized for this use
case.

Moreover, because we are in full control of the resolution,
we can support TS `paths` aliases, where not all imports come from
`node_modules`. This is the case in some CLI projects where there are
compiled libraries that are stored locally in a `dist` folder.
See //FW-1210.

PR Close #29643
AndrewKushnir pushed a commit that referenced this pull request Apr 29, 2019
By passing a `pathMappings` configuration (a subset of the
`ts.CompilerOptions` interface), we can instuct ngcc to process
additional paths outside the `node_modules` folder.

PR Close #29643
AndrewKushnir pushed a commit that referenced this pull request Apr 29, 2019
)

For UMD/RequireJS support we will need to have multiple
`DependencyHost` implementations. This commit  prepares the
ground for that.

PR Close #29643
AndrewKushnir pushed a commit that referenced this pull request Apr 29, 2019
This commit introduces a new interface, which abstracts access
to the underlying `FileSystem`. There is initially one concrete
implementation, `NodeJsFileSystem`, which is simply wrapping the
`fs` library of NodeJs.

Going forward, we can provide a `MockFileSystem` for test, which
should allow us to stop using `mock-fs` for most of the unit tests.
We could also implement a `CachedFileSystem` that may improve the
performance of ngcc.

PR Close #29643
AndrewKushnir pushed a commit that referenced this pull request Apr 29, 2019
@angular-automatic-lock-bot
Copy link
Copy Markdown

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.

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 area: core Issues related to the framework runtime cla: yes effort2: days feature Label used to distinguish feature request from other issues freq2: medium refactoring Issue that involves refactoring or code-cleanup risk: medium target: major This PR is targeted for the next major release type: bug/fix workaround2: non-obvious

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants