fix(core): avoid migration error when non-existent symbol is imported#36367
Closed
devversion wants to merge 1 commit intoangular:masterfrom
Closed
fix(core): avoid migration error when non-existent symbol is imported#36367devversion wants to merge 1 commit intoangular:masterfrom
devversion wants to merge 1 commit intoangular:masterfrom
Conversation
devversion
added
area: migrations
crisbeto
approved these changes
Apr 1, 2020
Member
crisbeto
left a comment
crisbeto
left a comment
There was a problem hiding this comment.
The changes LGTM, but I'm wondering whether we couldn't move the resolve call into a common place (e.g. where the new error was added). It seems like we're repeating it in several places and we could forget to add it in any new migrations.
devversion
force-pushed
the
fix/migrations-do-not-throw-root-dirs
branch
from
6573756 to
e5ce569
Compare
Member
Author
|
@crisbeto Agreed. I wanted to reduce the overall TS program duplication separately, but now did it in this PR. Can you please have another look? |
devversion
force-pushed
the
fix/migrations-do-not-throw-root-dirs
branch
from
e5ce569 to
f838c26
Compare
devversion
added
action: merge
Contributor
|
@devversion Can you rebase? |
kara
added
the
action: cleanup
devversion
force-pushed
the
fix/migrations-do-not-throw-root-dirs
branch
from
f838c26 to
9101561
Compare
In rare cases a project with configured `rootDirs` that has imports to non-existent identifiers could fail in the migration. This happens because based on the application code, the migration could end up trying to resolve the `ts.Symbol` of such non-existent identifiers. This isn't a problem usually, but due to a upstream bug in the TypeScript compiler, a runtime error is thrown. This is because TypeScript is unable to compute a relative path from the originating source file to the imported source file which _should_ provide the non-existent identifier. An issue for this has been reported upstream: microsoft/TypeScript#37731. The issue only surfaces since our migrations don't provide an absolute base path that is used for resolving the root directories. To fix this, we ensure that we never use relative paths when parsing tsconfig files. More details can be found in the TS issue. Fixes angular#36346.
devversion
force-pushed
the
fix/migrations-do-not-throw-root-dirs
branch
from
9101561 to
d7cfc9e
Compare
Member
Author
|
@kara Done. Thx for reviewing! Waiting for the PR to turn green. |
devversion
added
merge: caretaker note
Member
Author
|
Caretaker: Can you please run presubmit for this? Thank you! |
Contributor
kara
pushed a commit
that referenced
this pull request
Apr 6, 2020
…#36367) In rare cases a project with configured `rootDirs` that has imports to non-existent identifiers could fail in the migration. This happens because based on the application code, the migration could end up trying to resolve the `ts.Symbol` of such non-existent identifiers. This isn't a problem usually, but due to a upstream bug in the TypeScript compiler, a runtime error is thrown. This is because TypeScript is unable to compute a relative path from the originating source file to the imported source file which _should_ provide the non-existent identifier. An issue for this has been reported upstream: microsoft/TypeScript#37731. The issue only surfaces since our migrations don't provide an absolute base path that is used for resolving the root directories. To fix this, we ensure that we never use relative paths when parsing tsconfig files. More details can be found in the TS issue. Fixes #36346. PR Close #36367
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In rare cases a project with configured
rootDirsthat has imports tonon-existent identifiers could fail in the migration.
This happens because based on the application code, the migration could
end up trying to resolve the
ts.Symbolof such non-existentidentifiers. This isn't a problem usually, but due to a upstream bug
in the TypeScript compiler, a runtime error is thrown.
This is because TypeScript is unable to compute a relative path from the
originating source file to the imported source file which should
provide the non-existent identifier. An issue for this has been reported
upstream: microsoft/TypeScript#37731. The
issue only surfaces since our migrations don't provide an absolute base
path that is used for resolving the root directories.
To fix this, we ensure that we never use relative paths when parsing
tsconfig files. More details can be found in the TS issue.
Fixes #36346.