feat(ivy): ngtsc template sourcemaps - #28055
petebacondarwin wants to merge 17 commits into
Conversation
d2a34f1 to
23fb85f
Compare
|
You can preview d2a34f1 at https://pr28055-d2a34f1.ngbuilds.io/. |
|
You can preview 23fb85f at https://pr28055-23fb85f.ngbuilds.io/. |
62f08cd to
8ed8779
Compare
|
You can preview 8ed8779 at https://pr28055-8ed8779.ngbuilds.io/. |
There was a problem hiding this comment.
Is there a way we can pass the starting line/col position in along with the range (since TypeScript knows it)?
There was a problem hiding this comment.
I would prefer we not make this change, actually. I still weighing the benefits but right now I'm inclined to avoid the use of spying in tests, in favor of proper interface/implementation decoupling.
I've found having tests based on spies negates a large part of the benefits of static typing. In particular, it's incredibly powerful to be able to make an API change in some part of the codebase and see, via TS errors, all of the sites where the API is consumed that need to be updated. Having spies in tests (which are not strongly typed to the API they're spying on) defeats this approach, and leads to unexpected and sometimes hard to debug failures.
I encountered this recently when changing some of the ngtsc APIs that were consumed by ngcc. I was able to fix usages inside ngcc fairly easily, but the tests took me much longer because I wasn't able to rely on TypeScript to resolve any API mismatches.
I would favor a Jitter service we could create a proper fake implementation for instead of organizing the functions to be easily spied on.
There was a problem hiding this comment.
Let me look into that.
There was a problem hiding this comment.
Can we get rid of sourceMapUrl above?
There was a problem hiding this comment.
Could this commit be merged into the previous? Also, same comment about sourceMapUrl.
There was a problem hiding this comment.
I can squash the commit but these two commits are doing different things.
In this case the sourceMapUrl is different to the line being changed. It is the URL of the source template, whereas this line is defining the URL of the generated factory.
|
@alxhub I have refactored the lexer so that you pass in the line and col when providing a range, so that avoids having to advance over the whole file! See ea51273600778af44d7e232955c1f4aa7610165e and 09b13f7def03935195291659a8f2a37ae364e347 |
|
@alxhub - regarding backporting the VE stuff to patch... let's get this PR into shape and land it. Then I will create a new PR for patch with the best bits cherry-picked. |
|
Fixed up, rebased and ready for @alxhub to PTAL. I moved all the fixup commits to directly after the commit they are fixing, to ensure that they will squash correctly when merged into master. So when reviewing you might need to look down the list of commits to find the fixups. |
|
Hmm, except the AOT tests are failing... |
There was a problem hiding this comment.
Why does it come up? Which files are generated that are not ignored?
There was a problem hiding this comment.
When debugging a Bazel node test, VS code notices that the whole Bazel test output code, through whose source you are debugging has a git repository of its own - but it seems to think that there are thousands of changes...
There was a problem hiding this comment.
Is this change (true --> false) intentional?
There was a problem hiding this comment.
Yes, this boolean flag turns on SourceMap generation. :-)
There was a problem hiding this comment.
Can you add a comment with the parameter name of the flag? , /* enableSourceMaps */ true
There was a problem hiding this comment.
Done - but not in this file (because this code got moved) - see https://github.com/angular/angular/pull/28055/files#diff-a6e5b540987d60524fba9e53e41f2920R175
|
@petebacondarwin Looks like this just needs a rebase |
This warning pops up every time you try to run a node debug session via bazel. It is not important.
This commit consolidates the options that can modify the parsing of text (e.g. HTML, Angular templates, CSS, i18n) into an AST for further processing into a single `options` hash. This makes the code cleaner and more readable, but also enables us to support further options to parsing without triggering wide ranging changes to code that should not be affected by these new options. Specifically, it will let us pass information about the placement of a template that is being parsed in its containing file, which is essential for accurate SourceMap processing.
When we added the strict null checks, the lexer had some `!` operators added to prevent the compilation from failing. This commit resolves this problem correctly and removes the hacks. Also the comment ``` // Note: this is always lowercase! ``` has been removed as it is no longer true. See angular#24571
|
@petebacondarwin FYI I rebased the PR. |
|
@petebacondarwin This PR can't be merged because it does not squash cleanly. Could you squash and force push, and I will merge it. |
The lexer that does the tokenizing can now process only a part the source string, by passing a `range` property in the `options` argument. The locations of the nodes that are tokenized will now take into account the position of the span in the context of the original source string. This `range` option is, in turn, exposed from the template parser as well. Being able to process parts of files helps to enable SourceMap support when compiling inline component templates.
In order to support source mapping of templates, we need to be able to tokenize the template in its original context. When the template is defined inline as a JavaScript string in a TS/JS source file, the tokenizer must be able to handle string escape sequences, such as `\n` and `\"` as they appear in the original source file. This commit teaches the lexer how to unescape these sequences, but only when the `escapedString` option is set to true. Otherwise there is no change to the tokenizing behaviour.
There were a number of typos and some of the sentences did not read well.
When we resolve a component `templateUrl` we copy the contents of the resolved template file into the `template` property. Previously we would then remove the `templateUrl` to indicate that the component has been resolved. But this meant that we no longer had access to the URL of the original template file. This is essential for diagnostics messages about the template compilation. Now the existence of the `template` property overrides the existence of `templateUrl`, which allows us to keep the `templateUrl` property.
When testing JIT code, it is useful to be able to access the generated JIT source. Previously this is done by spying on the global `Function` object, to capture the code when it is being evaluated. This is problematic because you can only capture the body of the function, and not the arguments, which messes up line and column positions for source mapping for instance. Now the code that generates and then evaluates JIT code is wrapped in a `JitEvaluator` class, making it possible to provide a mock implementation that can capture the generated source of the function passed to `executeFunction(fn: Function, args: any[])`.
… lines Previously the call to `extractSourceMap()` would only work if the `//#sourceMappingURL ...` was the last line of the file. This doesn't work if the code is JIT evaluated as the comment is actually the last line in the body of a function, wrapped by curly-braces.
Previously JIT compiled components did not use the correct URL if the template was resolved from a `templateUrl`.
Previously the generated code was being mapped to the `templateUrl` value.
There are some differences in how ivy maps template source compared to View Engine. In this commit we recreate the View Engine tests for ivy.
When tokenizing markup (e.g. HTML) element attributes can have quoted or unquoted values (e.g. `a=b` or `a="b"`). The `ATTR_VALUE` tokens were capturing the quotes, which was inconsistent and also affected source-mapping. Now the tokenizer captures additional `ATTR_QUOTE` tokens, which the HTML related parsers understand and factor into their token parsing.
…ngs to output AST The `convertActionBinding()` now accepts an optional `baseSourceSpan`, which is the start point of the action expression being converted in the original source code. This is used to compute the original position of the output AST nodes.
When template bindings are being parsed the event handlers were receiving a source span that included the whole attribute. Now they get a span that is focussed on the handler itself.
During analysis, the `ComponentDecoratorHandler` passes the component template to the `parseTemplate()` function. Previously, there was little or no information about the original source file, where the template is found, passed when calling this function. Now, we correctly compute the URL of the source of the template, both for external `templateUrl` and in-line `template` cases. Further in the in-line template case we compute the character range of the template in its containing source file; *but only in the case that the template is a simple string literal*. If the template is actually a dynamic value like an interpolated string or a function call, then we do not try to add the originating source file information. The translator that converts Ivy AST nodes to TypeScript now adds these template specific source mappings, which account for the file where the template was found, to the templates to support stepping through the template creation and update code when debugging an Angular application. Note that some versions of TypeScript have a bug which means they cannot support external template source-maps. We check for this via the `canSourceMapExternalTemplates()` helper function and avoid trying to add template mappings to external templates if not supported.
|
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. |

There is a bug in TypeScript microsoft/TypeScript#29300 that prevents the
templateUrlURL from working correctly. It is fixed in TS 3.3. We have a check to fail gracefully if aversion of TS is running that does not include the fix.
For inline templates, only string literals are supported.
The actual mapping from the generated ivy code to the templates needs some work to improve its accuracy and resolution.