fix(ngcc): handle missing sources and out of range mappings when flattening source-maps by petebacondarwin · Pull Request #35718 · angular/angular · GitHub
Skip to content

fix(ngcc): handle missing sources and out of range mappings when flattening source-maps - #35718

Closed
petebacondarwin wants to merge 4 commits into
angular:masterfrom
petebacondarwin:ngcc-source-map-regression
Closed

petebacondarwin wants to merge 4 commits into
angular:masterfrom
petebacondarwin:ngcc-source-map-regression

Conversation

@petebacondarwin

@petebacondarwin petebacondarwin commented Feb 27, 2020 •

Copy link
Copy Markdown
Contributor

This PR has two commits that fix issues with source-maps that are badly formatted.


If a package has a source-map but it does not provide the actual content of the sources, then the source-map flattening was crashing.

Now we ignore such mappings that have no source since we are not able to compute the merged mapping if there is no source file.


Previously when rendering flattened source-maps, it was assumed that no mapping would come from a line that is outside the lines of the actual source content. It turns out this is not a valid assumption.

Now the code that renders flattened source-maps will handle such mappings, with the additional benefit that the rendered source-map will only contain mapping lines up to the last mapping, rather than a mapping line for every content line.


Fixes #35709

@petebacondarwin petebacondarwin added type: bug/fix state: WIP freq3: high regression Indicates than the issue relates to something that worked in a previous version target: patch This PR is targeted for the next patch release risk: high comp: ngcc labels Feb 27, 2020
@ngbot ngbot Bot added this to the needsTriage milestone Feb 27, 2020
@petebacondarwin
petebacondarwin force-pushed the ngcc-source-map-regression branch from 5eda8d1 to fc0e704 Compare February 27, 2020 13:11
@petebacondarwin
petebacondarwin marked this pull request as ready for review February 27, 2020 14:15
@petebacondarwin petebacondarwin changed the title fix(ngcc): handle missing sources when flattening source-maps fix(ngcc): handle missing sources and out of range mappings when flattening source-maps Feb 27, 2020
@petebacondarwin petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Feb 27, 2020

@gkalpak gkalpak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 27, 2020
@petebacondarwin

Copy link
Copy Markdown
Contributor Author

Comment thread packages/compiler-cli/ngcc/src/sourcemaps/source_file.ts Outdated
Comment thread packages/compiler-cli/ngcc/src/sourcemaps/source_file.ts Outdated
@gkalpak

gkalpak commented Feb 27, 2020

Copy link
Copy Markdown
Member

BTW, it would be a good idea to run this against angular/ngcc-validation before merging to make sure there are no other regressions.

@gkalpak gkalpak added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Feb 27, 2020
If a package has a source-map but it does not provide
the actual content of the sources, then the source-map
flattening was crashing.

Now we ignore such mappings that have no source
since we are not able to compute the merged
mapping if there is no source file.

Fixes angular#35709
…-maps

Previously when rendering flattened source-maps, it was assumed that no
mapping would come from a line that is outside the lines of the actual
source content. It turns out this is not a valid assumption.

Now the code that renders flattened source-maps will handle such
mappings, with the additional benefit that the rendered source-map
will only contain mapping lines up to the last mapping, rather than a
mapping line for every content line.

Fixes angular#35709
@petebacondarwin
petebacondarwin force-pushed the ngcc-source-map-regression branch from ef877a2 to 9ae9237 Compare February 27, 2020 19:50
@petebacondarwin

petebacondarwin commented Feb 27, 2020 •

Copy link
Copy Markdown
Contributor Author

Addressed the feedback. Now checking it against ngcc-validation repo.

ngcc-vaildation repo test was green: https://app.circleci.com/github/angular/ngcc-validation/pipelines?branch=pull%2F943

Comment thread packages/compiler-cli/ngcc/src/rendering/source_maps.ts Outdated
Source-maps in the wild could be badly formatted,
causing the source-map flattening processing to fail
unexpectedly. Rather than causing the whole of ngcc
to crash, we gracefully fallback to just returning the
generated source-map instead.
@petebacondarwin
petebacondarwin force-pushed the ngcc-source-map-regression branch from 9ae9237 to b60e604 Compare February 27, 2020 20:38
@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Feb 27, 2020
matsko pushed a commit that referenced this pull request Feb 27, 2020
If a package has a source-map but it does not provide
the actual content of the sources, then the source-map
flattening was crashing.

Now we ignore such mappings that have no source
since we are not able to compute the merged
mapping if there is no source file.

Fixes #35709

PR Close #35718
matsko pushed a commit that referenced this pull request Feb 27, 2020
…-maps (#35718)

Previously when rendering flattened source-maps, it was assumed that no
mapping would come from a line that is outside the lines of the actual
source content. It turns out this is not a valid assumption.

Now the code that renders flattened source-maps will handle such
mappings, with the additional benefit that the rendered source-map
will only contain mapping lines up to the last mapping, rather than a
mapping line for every content line.

Fixes #35709

PR Close #35718
matsko pushed a commit that referenced this pull request Feb 27, 2020
…35718)

Source-maps in the wild could be badly formatted,
causing the source-map flattening processing to fail
unexpectedly. Rather than causing the whole of ngcc
to crash, we gracefully fallback to just returning the
generated source-map instead.

PR Close #35718
@matsko matsko closed this in 72c4fda Feb 27, 2020
matsko pushed a commit that referenced this pull request Feb 27, 2020
…-maps (#35718)

Previously when rendering flattened source-maps, it was assumed that no
mapping would come from a line that is outside the lines of the actual
source content. It turns out this is not a valid assumption.

Now the code that renders flattened source-maps will handle such
mappings, with the additional benefit that the rendered source-map
will only contain mapping lines up to the last mapping, rather than a
mapping line for every content line.

Fixes #35709

PR Close #35718
matsko pushed a commit that referenced this pull request Feb 27, 2020
…35718)

Source-maps in the wild could be badly formatted,
causing the source-map flattening processing to fail
unexpectedly. Rather than causing the whole of ngcc
to crash, we gracefully fallback to just returning the
generated source-map instead.

PR Close #35718
@petebacondarwin
petebacondarwin deleted the ngcc-source-map-regression branch February 28, 2020 12:04
@angular-automatic-lock-bot

Copy link
Copy Markdown

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Mar 30, 2020
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 cla: yes freq3: high regression Indicates than the issue relates to something that worked in a previous version risk: high target: patch This PR is targeted for the next patch release type: bug/fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Angular 9.0.3 ngcc fails to process ng2-dragula

5 participants