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

ngcc: source map flattening #35132

Conversation

@petebacondarwin
Copy link
Member

petebacondarwin commented Feb 3, 2020

The library used by ngcc to update the source files (MagicString) is able
to generate a source-map but it is not able to account for any previous
source-map that the input text is already associated with.

There have been various attempts to fix this but none have been very
successful, since it is not a trivial problem to solve.

This PR contains a novel approach that is able to load up a tree of
source-files connected by source-maps and flatten them down into a single
source-map that maps directly from the final generated file to the original
sources referenced by the intermediate source-maps.

(Note that there are probably (definitely) some terrible performance characteristics in this PR. But I wanted to go for correctness before stressing about speed.)

FW-1558

@googlebot googlebot added the cla: yes label Feb 3, 2020
@pullapprove pullapprove bot requested review from filipesilva and JoostK Feb 3, 2020
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngcc-source-mapping-FW-1558 branch 2 times, most recently from 90969cf to 1431fef Feb 3, 2020
@mhevery mhevery added the comp: ngcc label Feb 4, 2020
@ngbot ngbot bot added this to the needsTriage milestone Feb 4, 2020
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngcc-source-mapping-FW-1558 branch 4 times, most recently from 157328a to 1ca79cd Feb 6, 2020
@pullapprove pullapprove bot requested a review from mhevery Feb 10, 2020
@petebacondarwin petebacondarwin marked this pull request as ready for review Feb 10, 2020
Copy link
Member

filipesilva left a comment

I know you mentioned you wanted to go for correctness before speed, but can you check what the perf difference is? We shouldn't merge this yet if it makes everyone's first build 3x as long.

@petebacondarwin

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Feb 11, 2020

@filipesilva absolutely!

@JoostK

This comment has been minimized.

Copy link
Member

JoostK commented Feb 11, 2020

Additionally, could this work be avoided if source map generation is disabled?

@petebacondarwin

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Feb 11, 2020

@JoostK - what does that actually mean in practical terms? Are you only talking about during CLI integration?

The problem we have is that if there is already a source-map for the original file and we don't do anything then the result is a very broken source-map becomes available to the user of the source generated by ngcc.

By the way, if there is no original source-map then this would short-circuit and be super quick any way.

@JoostK

This comment has been minimized.

Copy link
Member

JoostK commented Feb 11, 2020

@petebacondarwin Sorry, I was a little vague. What I meant to say is that when an app is built without source maps enabled (the TS sourcemap option is false or sourceMap is disabled in the CLI configuration), the user is likely not interested in source maps coming from ngcc as well. Therefore, the additional work of merging source maps in ngcc may be for nothing. I see your point when there's already an existing sourcemap, in which case not merging source maps would result in an incorrect result (probably just as broken as what is generated by ngcc as of today)

@petebacondarwin

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Feb 11, 2020

Also we have to be careful in monorepo scenarios where the ngcc generated files might be used in different compilations that are configured differently. So I think assuming that there is no major performance issues it is safer to always process the source file...

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngcc-source-mapping-FW-1558 branch 2 times, most recently from f796455 to 33734df Feb 13, 2020
@petebacondarwin

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Feb 13, 2020

I have now tweaked this algorithm a bit more and I believe it is working as desired.

Regarding performance, I ran ngcc against the entire AIO node_modules folder (all package and all formats) and compared the time taken on master and this PR. The average time on master was 140 seconds. The average time for this PR was 149 seconds. So while there is an increase in processing time it is around only 6%, which I argue is acceptable. It is possible that we can do some further performance tuning to improve things further later on.

@petebacondarwin petebacondarwin changed the title Ngcc source mapping fw 1558 ngcc: source map flattening Feb 13, 2020
@filipesilva

This comment has been minimized.

Copy link
Member

filipesilva commented Feb 13, 2020

Yeah 6% is pretty much within just normal variance even.

Copy link
Member

filipesilva left a comment

LGTM as far as the CLI and perf are concerned, also LGTM from a dev-infra review perspective.

Copy link
Member

JoostK left a comment

I haven't been through the tests yet, but this is awesome work. Hats off to you! 👏

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngcc-source-mapping-FW-1558 branch from 91bcda7 to 03db6d5 Feb 16, 2020
@kara kara requested a review from JoostK Feb 24, 2020
@JoostK
JoostK approved these changes Feb 24, 2020
Copy link
Member

JoostK left a comment

The changes in this PR LGTM, but I just realized something:

When rendering the generated statements to JavaScript code to insert into the JS bundle, any source mapping information present in the statements is not retained as all statement outputs are concatenated into a single string:

private renderStatements(
sourceFile: ts.SourceFile, statements: Statement[], imports: ImportManager): string {
const printStatement = (stmt: Statement) =>
this.srcFormatter.printStatement(stmt, sourceFile, imports);
return statements.map(printStatement).join('\n');
}

Although this PR should address the unmapped bytes problem as reported in FW-1558 (did we actually verify that?), it does not improve the granularity of the source maps of the generated code.

@petebacondarwin

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Feb 24, 2020

There are a few issues..

  1. the previous source-map basically dropped all the references to the previous original source files, which is what the JIRA issue is primarily complaining about.
  2. the mappings in that source-map were only showing the lowest resolution between the merged source maps, which results in them being of minimal use.
  3. the output from ngcc compilation is indeed just a single string with no additional source-map information, which means that we do not benefit from things like template source map information.

This PR fixes both 1 and 2. I have checked both. The resulting source-map will show all the original mappings from TS to JS - the new ngcc mappings to ivy definition fields are in the correct place but are not high definition.

Number 3 is a more complex issue, and I feel that given that ngcc is only providing backward compatibility to libraries, and hopefully should only be for a limited amount of time, it may not be worth investing time in.

@JoostK

This comment has been minimized.

Copy link
Member

JoostK commented Feb 24, 2020

This PR fixes both 1 and 2. I have checked both.

Excellent! 💯

Number 3 is a more complex issue, and I feel that given that ngcc is only providing backward compatibility to libraries, and hopefully should only be for a limited amount of time, it may not be worth investing time in.

Agreed, just wanted to make sure this does indeed solve the unmapped bytes problem.

@petebacondarwin

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Feb 24, 2020

Here is a link to a document that gives a high level explanation of what is going on here: https://docs.google.com/document/d/1x1laiDVFgJtpiw49ozwr91LG7hqohytWDiptJ93nJcE

@petebacondarwin petebacondarwin removed the request for review from mhevery Feb 24, 2020
Copy link
Member

gkalpak left a comment

Nice! +1 for the ASCII art 😛

The library used by ngcc to update the source files (MagicString) is able
to generate a source-map but it is not able to account for any previous
source-map that the input text is already associated with.

There have been various attempts to fix this but none have been very
successful, since it is not a trivial problem to solve.

This commit contains a novel approach that is able to load up a tree of
source-files connected by source-maps and flatten them down into a single
source-map that maps directly from the final generated file to the original
sources referenced by the intermediate source-maps.
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngcc-source-mapping-FW-1558 branch from a75bb52 to 4187975 Feb 26, 2020
@mhevery mhevery closed this in df816c9 Feb 26, 2020
mhevery added a commit that referenced this pull request Feb 26, 2020
The library used by ngcc to update the source files (MagicString) is able
to generate a source-map but it is not able to account for any previous
source-map that the input text is already associated with.

There have been various attempts to fix this but none have been very
successful, since it is not a trivial problem to solve.

This commit contains a novel approach that is able to load up a tree of
source-files connected by source-maps and flatten them down into a single
source-map that maps directly from the final generated file to the original
sources referenced by the intermediate source-maps.

PR Close #35132
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

angular-automatic-lock-bot bot commented Mar 28, 2020

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.