Skip to content

feat(ivy): ngtsc template sourcemaps - #28055

Closed
petebacondarwin wants to merge 17 commits into
angular:masterfrom
petebacondarwin:ngtsc-template-sourcemaps
Closed

petebacondarwin wants to merge 17 commits into
angular:masterfrom
petebacondarwin:ngtsc-template-sourcemaps

Conversation

@petebacondarwin

@petebacondarwin petebacondarwin commented Jan 10, 2019

Copy link
Copy Markdown
Contributor

There is a bug in TypeScript microsoft/TypeScript#29300 that prevents the templateUrl URL from working correctly. It is fixed in TS 3.3. We have a check to fail gracefully if a
version 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.

@petebacondarwin petebacondarwin added state: WIP effort2: days freq3: high area: core Issues related to the framework runtime target: major This PR is targeted for the next major release comp: ivy risk: medium labels Jan 10, 2019
@petebacondarwin
petebacondarwin requested review from a team January 10, 2019 22:20
@ngbot ngbot Bot added this to the needsTriage milestone Jan 10, 2019
@petebacondarwin
petebacondarwin force-pushed the ngtsc-template-sourcemaps branch from d2a34f1 to 23fb85f Compare January 10, 2019 22:23
@mary-poppins

Copy link
Copy Markdown

You can preview d2a34f1 at https://pr28055-d2a34f1.ngbuilds.io/.

@mary-poppins

Copy link
Copy Markdown

You can preview 23fb85f at https://pr28055-23fb85f.ngbuilds.io/.

@petebacondarwin
petebacondarwin force-pushed the ngtsc-template-sourcemaps branch 2 times, most recently from 62f08cd to 8ed8779 Compare January 11, 2019 08:38
@mary-poppins

Copy link
Copy Markdown

You can preview 8ed8779 at https://pr28055-8ed8779.ngbuilds.io/.

@petebacondarwin
petebacondarwin requested review from a team January 14, 2019 22:20

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.

Is there a way we can pass the starting line/col position in along with the range (since TypeScript knows it)?

Comment thread packages/compiler/src/ml_parser/lexer.ts Outdated

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.

This PR does everything! 😆

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.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Let me look into that.

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.

Can we get rid of sourceMapUrl above?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah!

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.

Could this commit be merged into the previous? Also, same comment about sourceMapUrl.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@petebacondarwin

Copy link
Copy Markdown
Contributor Author

@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

@petebacondarwin

Copy link
Copy Markdown
Contributor Author

@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.

@petebacondarwin

Copy link
Copy Markdown
Contributor Author

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.

@petebacondarwin

Copy link
Copy Markdown
Contributor Author

Hmm, except the AOT tests are failing...

@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.

This is so great 👌 ❤️ 🎊

Comment thread .vscode/settings.json Outdated

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.

Why does it come up? Which files are generated that are not ignored?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

screenshot 2019-02-11 at 19 58 17

Comment thread packages/compiler/src/render3/r3_jit.ts Outdated

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.

Is this change (true --> false) intentional?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, this boolean flag turns on SourceMap generation. :-)

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.

Can you add a comment with the parameter name of the flag? , /* enableSourceMaps */ true

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done - but not in this file (because this code got moved) - see https://github.com/angular/angular/pull/28055/files#diff-a6e5b540987d60524fba9e53e41f2920R175

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.

Unnecessary newline?

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.

🎉

@kara

kara commented Feb 12, 2019

Copy link
Copy Markdown
Contributor

@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
@kara

kara commented Feb 12, 2019

Copy link
Copy Markdown
Contributor

@petebacondarwin FYI I rebased the PR.

presubmit

@mhevery

mhevery commented Feb 13, 2019

Copy link
Copy Markdown
Contributor

@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.
@angular-automatic-lock-bot

Copy link
Copy Markdown

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.

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 area: core Issues related to the framework runtime cla: yes effort2: days freq3: high risk: medium target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants