fix(compiler-cli): handle pseudo cycles in inline source-maps #40435
Conversation
This file contains 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
petebacondarwin
added
target: patch
petebacondarwin
force-pushed
the
source-maps-self-referencin-inline-issue-40408
branch
from
d68569a
to
b6af801
Compare
petebacondarwin
commented
Jan 15, 2021
petebacondarwin
force-pushed
the
source-maps-self-referencin-inline-issue-40408
branch
3 times, most recently
from
c0a2aeb
to
02ad8f0
Compare
petebacondarwin
added
the
action: review
petebacondarwin
force-pushed
the
source-maps-self-referencin-inline-issue-40408
branch
2 times, most recently
from
9ddfdee
to
806ad70
Compare
petebacondarwin
force-pushed
the
source-maps-self-referencin-inline-issue-40408
branch
3 times, most recently
from
b62779d
to
a79bfe1
Compare
JoostK
reviewed
Jan 17, 2021
packages/compiler-cli/src/ngtsc/sourcemaps/src/source_file_loader.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/sourcemaps/src/source_file_loader.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/sourcemaps/src/source_file_loader.ts
Outdated
Show resolved
Hide resolved
JoostK
reviewed
Jan 20, 2021
packages/compiler-cli/src/ngtsc/sourcemaps/src/source_file_loader.ts
Outdated
Show resolved
Hide resolved
Comment on lines
+152
to
+154
| // The source file was provided inline and its contents did not include an inline source-map. | ||
| // So we don't try to load an external source-map from the file-system, since this can lead to | ||
| // invalid circular dependencies. |
There was a problem hiding this comment.
Could there be legitimate cases that will now hit this case and fail to load? i.e. a source map that references another source file inline, which has an external source map comment to a source map file that actually exists? If I understand correctly, that scenario will be rejected now where the external source map file is refused to load.
There was a problem hiding this comment.
I thought about this. And of course it is feasible that this might happen. But I believe that in practice it will not: our library builds will not do this. Further, I think that there is just as good a chance that trying to load something from the disk from something that was inline is just as likely to produce a failure of some kind.
The bottom line is that anything is possible, I don't think it is possible to handle everything and most of these corner cases will be wrong in some way or another anyway whatever we do!
When a source-map has an inline source, any source-map linked from that source should only be loaded if itself is also inline; it should not attempt to load a source-map from the file-system. Otherwise we can find ourselves with inadvertent infinite cyclic dependencies. For example, if a transpiler takes a file (e.g. index.js) and generates a new file overwriting the original file - capturing the original source inline in the new source-map (index.js.map) - the source file loader might read the inline original file (also index.js) and then try to load the `index.js.map` file from disk - ad infinitum. Note that the first call to `loadSourceFile()` is special, since you can pass in the source-file and source-map contents directly as in-memory strrngs. This is common if the transpiler has just generated these and has not yet written them to disk. When the contents are passed into `loadSourceFile()` directly, they are not treated as "inline" for the purposes described above since there is no chance of these "in-memory" source and source-map contents being caught up in a cyclic dependency. Fixes angular#40408
petebacondarwin
force-pushed
the
source-maps-self-referencin-inline-issue-40408
branch
from
fa4d239
to
965c0e7
Compare
JoostK
approved these changes
Jan 21, 2021
petebacondarwin
added
action: merge
jessicajaniuk
pushed a commit
that referenced
this pull request
Jan 21, 2021
When a source-map has an inline source, any source-map linked from that source should only be loaded if itself is also inline; it should not attempt to load a source-map from the file-system. Otherwise we can find ourselves with inadvertent infinite cyclic dependencies. For example, if a transpiler takes a file (e.g. index.js) and generates a new file overwriting the original file - capturing the original source inline in the new source-map (index.js.map) - the source file loader might read the inline original file (also index.js) and then try to load the `index.js.map` file from disk - ad infinitum. Note that the first call to `loadSourceFile()` is special, since you can pass in the source-file and source-map contents directly as in-memory strrngs. This is common if the transpiler has just generated these and has not yet written them to disk. When the contents are passed into `loadSourceFile()` directly, they are not treated as "inline" for the purposes described above since there is no chance of these "in-memory" source and source-map contents being caught up in a cyclic dependency. Fixes #40408 PR Close #40435
petebacondarwin
deleted the
source-maps-self-referencin-inline-issue-40408
branch
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
When a source-map has an inline source, any source-map linked from
that source should only be loaded if itself is also inline; it should not
attempt to load a source-map from the file-system. Otherwise we can
find ourselves with inadvertent infinite cyclic dependencies.
For example if a transpiler takes a file (e.g. index.js) and generates
a new file overwriting the original file (but capturing the original
source inline in the new source-map (index.js.map) the source
file loader might read the inline original file (also index.js) and
then try to load the
index.js.mapfile from disk - ad infinitum.Fixes #40408