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

Monorepository import support #33828

Closed
wants to merge 2 commits into from

Conversation

@alxhub
Copy link
Contributor

alxhub commented Nov 14, 2019

❤️

@googlebot googlebot added the cla: yes label Nov 14, 2019
@alxhub alxhub marked this pull request as ready for review Nov 14, 2019
@alxhub alxhub requested a review from angular/fw-compiler as a code owner Nov 14, 2019
@JoostK
JoostK approved these changes Nov 14, 2019
Copy link
Member

JoostK left a comment

LGTM with a couple of requests.

new LogicalProjectStrategy(this.reflector, new LogicalFileSystem(this.rootDirs));
} else {
// Plain relative imports are all that's needed.
localImportStrategy = new RelativePathStrategy(this.reflector);

This comment has been minimized.

Copy link
@JoostK

JoostK Nov 14, 2019

Member

I think you should be able to replace the LocigalProjectStrategy with RelativePath strategy within ngcc, as I recall there being a TODO there and this new strategy looks to implement exactly that.

This comment has been minimized.

Copy link
@alxhub

alxhub Nov 15, 2019

Author Contributor

You're right that there is. But looking at this makes me wonder.

Right now LogicalProjectStrategy protects ngcc from emitting a relative import that points outside of the current entrypoint (and thus outside of the current package). This seems like it might be a reasonable assertion to make. I'll leave it as-is for now, and we can examine the tradeoffs here in another PR. I don't wanna couple this important fix to that decision.

filipesilva added a commit to filipesilva/dashboard that referenced this pull request Nov 15, 2019
@alxhub alxhub force-pushed the alxhub:ngtsc/mono/relative branch from 28499d9 to 87aa0f7 Nov 15, 2019
alxhub added 2 commits Nov 13, 2019
Previously, the compiler assumed that all TS files logically within a
project existed under one or more "root directories". If the TS compiler
option `rootDir` or `rootDirs` was set, they would dictate the root
directories in use, otherwise the current directory was used.

Unfortunately this assumption was unfounded - it's common for projects
without explicit `rootDirs` to import from files outside the current
working directory. In such cases the `LogicalProjectStrategy` would attempt
to generate imports into those files, and fail. This would lead to no
`ReferenceEmitStrategy` being able to generate an import, and end in a
compiler assertion failure.

This commit introduces a new strategy to use when there are no `rootDirs`
explicitly present, the `RelativePathStrategy`. It uses simpler, filesystem-
relative paths to generate imports, even to files above the current working
directory.

Fixes #33659
Fixes #33562
This commit adds the ability to change directories using the compiler's
internal filesystem abstraction. This is a prerequisite for writing tests
which are sensitive to the current working directory.

In addition to supporting the `chdir()` operation, this commit also fixes
`getDefaultLibLocation()` for mock filesystems to not assume `node_modules`
is in the current directory, but to resolve it similarly to how Node does
by progressively looking higher in the directory tree.
@alxhub alxhub force-pushed the alxhub:ngtsc/mono/relative branch from 87aa0f7 to 2662c77 Nov 18, 2019
@alxhub

This comment has been minimized.

Copy link
Contributor Author

alxhub commented Nov 19, 2019

Presubmit
No Ivy Presubmit needed; these changes don't actually affect g3.

@alxhub alxhub closed this in 5172074 Nov 19, 2019
alxhub added a commit that referenced this pull request Nov 19, 2019
…3828)

Previously, the compiler assumed that all TS files logically within a
project existed under one or more "root directories". If the TS compiler
option `rootDir` or `rootDirs` was set, they would dictate the root
directories in use, otherwise the current directory was used.

Unfortunately this assumption was unfounded - it's common for projects
without explicit `rootDirs` to import from files outside the current
working directory. In such cases the `LogicalProjectStrategy` would attempt
to generate imports into those files, and fail. This would lead to no
`ReferenceEmitStrategy` being able to generate an import, and end in a
compiler assertion failure.

This commit introduces a new strategy to use when there are no `rootDirs`
explicitly present, the `RelativePathStrategy`. It uses simpler, filesystem-
relative paths to generate imports, even to files above the current working
directory.

Fixes #33659
Fixes #33562

PR Close #33828
alxhub added a commit that referenced this pull request Nov 19, 2019
…33828)

This commit adds the ability to change directories using the compiler's
internal filesystem abstraction. This is a prerequisite for writing tests
which are sensitive to the current working directory.

In addition to supporting the `chdir()` operation, this commit also fixes
`getDefaultLibLocation()` for mock filesystems to not assume `node_modules`
is in the current directory, but to resolve it similarly to how Node does
by progressively looking higher in the directory tree.

PR Close #33828
alxhub added a commit that referenced this pull request Nov 19, 2019
…3828)

Previously, the compiler assumed that all TS files logically within a
project existed under one or more "root directories". If the TS compiler
option `rootDir` or `rootDirs` was set, they would dictate the root
directories in use, otherwise the current directory was used.

Unfortunately this assumption was unfounded - it's common for projects
without explicit `rootDirs` to import from files outside the current
working directory. In such cases the `LogicalProjectStrategy` would attempt
to generate imports into those files, and fail. This would lead to no
`ReferenceEmitStrategy` being able to generate an import, and end in a
compiler assertion failure.

This commit introduces a new strategy to use when there are no `rootDirs`
explicitly present, the `RelativePathStrategy`. It uses simpler, filesystem-
relative paths to generate imports, even to files above the current working
directory.

Fixes #33659
Fixes #33562

PR Close #33828
@lucienbertin lucienbertin mentioned this pull request Nov 27, 2019
3 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.