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.

Sign up for GitHub

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

Jump to bottom

fix(compiler-cli): output source-mappings for localized strings #38645

Closed
wants to merge 6 commits into from
Closed

fix(compiler-cli): output source-mappings for localized strings #38645

wants to merge 6 commits into from

Conversation

petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Aug 30, 2020 •
edited

Previously, localized strings had very limited or incorrect source-mapping
information available.

Now the i18n AST nodes and related output AST nodes include source-span
information about message-parts and placeholders - including closing tag
placeholders.

This information is then used when generating the final localized string
ASTs to ensure that the correct source-mapping is rendered.

See #38588 (comment)
Fixes #38711
Fixes #38694

@petebacondarwin petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer area: i18n target: patch This PR is targeted for the next patch release type: bug/fix labels Aug 31, 2020
@ngbot ngbot bot modified the milestone: needsTriage Aug 31, 2020
@petebacondarwin petebacondarwin marked this pull request as ready for review August 31, 2020 10:23
@pullapprove pullapprove bot requested a review from JoostK August 31, 2020 10:23
@petebacondarwin petebacondarwin force-pushed the i18n-localize-sourcemapping branch 2 times, most recently from 0ead48b to 40abce4 Compare August 31, 2020 15:28
@petebacondarwin petebacondarwin changed the title fix(compiler): output source-mappings for localized strings fix(compiler-cli): output source-mappings for localized strings Aug 31, 2020
JoostK
JoostK reviewed Aug 31, 2020
View reviewed changes
Copy link
Member

@JoostK JoostK left a comment

Choose a reason for hiding this comment

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

Typo in commit "move the MessagePiece classes into output_ast.ts""

"render the source-spans of each of piece" -> "render the source-spans of each piece"

packages/compiler-cli/src/ngtsc/translator/src/translator.ts Outdated Show resolved Hide resolved
packages/compiler-cli/src/ngtsc/translator/src/translator.ts Outdated Show resolved Hide resolved
packages/compiler-cli/test/ngtsc/template_mapping_spec.ts Outdated Show resolved Hide resolved
@alxhub
Copy link
Member

alxhub commented Sep 2, 2020

Presubmit

@petebacondarwin petebacondarwin mentioned this pull request Sep 3, 2020
Closed
@petebacondarwin
refactor(compiler): capture interpolation source-spans in expression …
a14225b 
…parser

The expression parser will split the expression up at the interpolation markers
into expressions and static strings. This commit also captures the positions of
these strings in the expression to be used in source-mapping later.
@petebacondarwin
refactor(compiler): track the closing source-span of TagPlaceholders
3e4dc4e 
The `TagPlaceholder` can contain children, in which case there are two source
spans of interest: the opening tag and the closing tag. This commit now allows
the closing tag source-span to be tracked, so that it can be used later in
source-mapping.
@petebacondarwin
refactor(compiler): move the MessagePiece classes into output_ast.ts
e427c63 
The `MessagePiece` and derived classes, `LiteralPiece` and `PlaceholderPiece`
need to be referenced in the `LocalizedString` output AST class, so that we
can render the source-spans of each piece.
@petebacondarwin
fix(compiler-cli): compute source-mappings for localized strings
7d515e2 
Previously, localized strings had very limited or incorrect source-mapping
information available.

Now the i18n AST nodes and related output AST nodes include source-span
information about message-parts and placeholders - including closing tag
placeholders.

This information is then used when generating the final localized string
ASTs to ensure that the correct source-mapping is rendered.

See angular#38588 (comment)
@petebacondarwin
fixup! fix(compiler-cli): compute source-mappings for localized strings
f45a4ef
@petebacondarwin petebacondarwin mentioned this pull request Sep 4, 2020
Closed
@petebacondarwin petebacondarwin removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Sep 4, 2020
@petebacondarwin petebacondarwin added the action: merge The PR is ready for merge by the caretaker label Sep 4, 2020
@atscott atscott added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels Sep 8, 2020
@atscott atscott closed this in bf31ef2 Sep 8, 2020
atscott pushed a commit that referenced this pull request Sep 8, 2020
@petebacondarwin @atscott
refactor(compiler): track the closing source-span of TagPlaceholders (#…
109555b 
…38645)

The `TagPlaceholder` can contain children, in which case there are two source
spans of interest: the opening tag and the closing tag. This commit now allows
the closing tag source-span to be tracked, so that it can be used later in
source-mapping.

PR Close #38645
atscott pushed a commit that referenced this pull request Sep 8, 2020
@petebacondarwin @atscott
refactor(compiler): move the MessagePiece classes into output_ast.ts (#…
7a6a061 
…38645)

The `MessagePiece` and derived classes, `LiteralPiece` and `PlaceholderPiece`
need to be referenced in the `LocalizedString` output AST class, so that we
can render the source-spans of each piece.

PR Close #38645
atscott pushed a commit that referenced this pull request Sep 8, 2020
@petebacondarwin @atscott
fix(compiler-cli): compute source-mappings for localized strings (#38645
7e0b3fd 
)

Previously, localized strings had very limited or incorrect source-mapping
information available.

Now the i18n AST nodes and related output AST nodes include source-span
information about message-parts and placeholders - including closing tag
placeholders.

This information is then used when generating the final localized string
ASTs to ensure that the correct source-mapping is rendered.

See #38588 (comment)

PR Close #38645
@atscott
Copy link
Contributor

atscott commented Sep 8, 2020

@petebacondarwin - There was a merge conflict with the 10.1.x branch so I changed the target to major. Could you create a new PR that targets patch only?

@petebacondarwin petebacondarwin mentioned this pull request Sep 8, 2020
Closed
@petebacondarwin petebacondarwin deleted the i18n-localize-sourcemapping branch September 8, 2020 20:36
@angular-automatic-lock-bot
Copy link

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 Oct 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JoostK JoostK JoostK approved these changes

@alxhub alxhub alxhub approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: i18n cla: yes target: major This PR is targeted for the next major release type: bug/fix
Projects
None yet
Milestone
needsTriage
5 participants
@petebacondarwin @alxhub @atscott @JoostK @googlebot