fix(compiler): invalid extraction text by petebacondarwin · Pull Request #39486 · angular/angular

petebacondarwin · 2020-10-29T09:50:47Z

AndrewKushnir

Thanks for the fix @petebacondarwin, it looks good (just left 1 minor comment) 👍

I think it'd be great if @alxhub or @atscott can have a quick look too, since they worked with the source spans recently and may have more context.

Thank you.

packages/compiler/src/parse_util.ts

atscott · 2020-10-30T23:19:05Z

Let's make it a party :) also adding @JoostK since he has done specific work on ICU source spans in #39072

atscott

LGTM, only some optional style nits. I don't have any more context than @AndrewKushnir would have on i18n source spans though :)

packages/compiler/src/ml_parser/lexer.ts

petebacondarwin · 2020-10-31T12:40:56Z

Thanks for the reviews @AndrewKushnir and @atscott !

packages/compiler/src/compiler_util/expression_converter.ts

packages/compiler/src/template_parser/binding_parser.ts

packages/compiler/test/render3/view/i18n_spec.ts

AndrewKushnir · 2020-11-01T20:23:56Z

Presubmit.

AndrewKushnir · 2020-11-02T21:55:19Z

@petebacondarwin FYI presubmit is successful (I'm removing the "presubmit" label).

…-span (#39486) In an i18n message, two placeholders next to each other must have an "empty" message-part to separate them. Previously, the source-span for this message-part was pointing to the wrong original location. This caused problems in the generated source-maps and lead to extracted i18n messages from being rendered incorrectly. PR Close #39486

…n`s (#39486) The lexer is able to skip leading trivia in the `start` location of tokens. This makes the source-span more friendly since things like elements appear to begin at the start of the opening tag, rather than at the start of any leading whitespace, which could include newlines. But some tooling requires the full source-span to be available, such as when tokenizing a text span into an Angular expression. This commit simply adds the `fullStart` location to the `ParseSourceSpan` class, and ensures that places where such spans are cloned, this property flows through too. PR Close #39486

…9486) This commit ensures that when leading whitespace is skipped by the tokenizer, the original start location (before skipping) is captured in the `fullStart` property of the token's source-span. PR Close #39486

…er source-spans (#39486) Tokenized text node may have leading whitespace skipped from their source-span. But the source-span is used to compute where there are interpolated blocks, resulting in placeholder nodes whose source-spans are offset by the amount of skipped characters. This fix uses the `fullStart` location of text source-spans for computing the source-span of placeholders, so that they are accurate. Fixes #39195 PR Close #39486

…-span In an i18n message, two placeholders next to each other must have an "empty" message-part to separate them. Previously, the source-span for this message-part was pointing to the wrong original location. This caused problems in the generated source-maps and lead to extracted i18n messages from being rendered incorrectly.

The lexer is able to skip leading trivia in the `start` location of tokens. This makes the source-span more friendly since things like elements appear to begin at the start of the opening tag, rather than at the start of any leading whitespace, which could include newlines. But some tooling requires the full source-span to be available, such as when tokenizing a text span into an Angular expression. This commit simply adds the `fullStart` location to the `ParseSourceSpan` class, and ensures that places where such spans are cloned, this property flows through too.

This commit ensures that when leading whitespace is skipped by the tokenizer, the original start location (before skipping) is captured in the `fullStart` property of the token's source-span.

…er source-spans Tokenized text node may have leading whitespace skipped from their source-span. But the source-span is used to compute where there are interpolated blocks, resulting in placeholder nodes whose source-spans are offset by the amount of skipped characters. This fix uses the `fullStart` location of text source-spans for computing the source-span of placeholders, so that they are accurate. Fixes angular#39195

…n`s (#39486) The lexer is able to skip leading trivia in the `start` location of tokens. This makes the source-span more friendly since things like elements appear to begin at the start of the opening tag, rather than at the start of any leading whitespace, which could include newlines. But some tooling requires the full source-span to be available, such as when tokenizing a text span into an Angular expression. This commit simply adds the `fullStart` location to the `ParseSourceSpan` class, and ensures that places where such spans are cloned, this property flows through too. PR Close #39486

…9486) This commit ensures that when leading whitespace is skipped by the tokenizer, the original start location (before skipping) is captured in the `fullStart` property of the token's source-span. PR Close #39486

…er source-spans (#39486) Tokenized text node may have leading whitespace skipped from their source-span. But the source-span is used to compute where there are interpolated blocks, resulting in placeholder nodes whose source-spans are offset by the amount of skipped characters. This fix uses the `fullStart` location of text source-spans for computing the source-span of placeholders, so that they are accurate. Fixes #39195 PR Close #39486

angular-automatic-lock-bot · 2020-12-20T16:37:51Z

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

petebacondarwin added type: bug/fix action: review The PR is still awaiting reviews from at least one requested reviewer area: i18n Issues related to localization and internationalization target: patch This PR is targeted for the next patch release engine: ivy labels Oct 29, 2020

ngbot bot added this to the needsTriage milestone Oct 29, 2020

google-cla bot added the cla: yes label Oct 29, 2020

pullapprove bot requested a review from alxhub October 29, 2020 09:50

petebacondarwin changed the title ~~fix invalid extraction text~~ fix(compiler): invalid extraction text Oct 29, 2020

petebacondarwin added the area: compiler Issues related to `ngc`, Angular's template compiler label Oct 29, 2020

petebacondarwin requested a review from AndrewKushnir October 30, 2020 10:47

AndrewKushnir approved these changes Oct 30, 2020

View reviewed changes

packages/compiler/src/parse_util.ts Outdated Show resolved Hide resolved

AndrewKushnir requested a review from atscott October 30, 2020 23:14

atscott requested a review from JoostK October 30, 2020 23:19

atscott approved these changes Oct 30, 2020

View reviewed changes

packages/compiler/src/ml_parser/lexer.ts Outdated Show resolved Hide resolved

packages/compiler/src/ml_parser/lexer.ts Outdated Show resolved Hide resolved

petebacondarwin force-pushed the invalid-extraction-text-issue-39195 branch from 715b90d to 9260b6c Compare October 31, 2020 12:39

JoostK requested changes Oct 31, 2020

View reviewed changes

packages/compiler/src/compiler_util/expression_converter.ts Outdated Show resolved Hide resolved

packages/compiler/src/template_parser/binding_parser.ts Outdated Show resolved Hide resolved

packages/compiler/test/render3/view/i18n_spec.ts Outdated Show resolved Hide resolved

JoostK approved these changes Oct 31, 2020

View reviewed changes

petebacondarwin added the action: presubmit The PR is in need of a google3 presubmit label Oct 31, 2020

AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Nov 2, 2020

petebacondarwin added the action: merge The PR is ready for merge by the caretaker label Nov 3, 2020

alxhub approved these changes Nov 3, 2020

View reviewed changes

petebacondarwin removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Nov 3, 2020

josephperrott force-pushed the invalid-extraction-text-issue-39195 branch from 9260b6c to 04a6d42 Compare November 4, 2020 21:59

petebacondarwin added 4 commits November 6, 2020 16:36

refactor(compiler): capture fullStart locations when tokenizing

a5ded82

This commit ensures that when leading whitespace is skipped by the tokenizer, the original start location (before skipping) is captured in the `fullStart` property of the token's source-span.

petebacondarwin force-pushed the invalid-extraction-text-issue-39195 branch from 04a6d42 to 18d2395 Compare November 6, 2020 16:36

petebacondarwin mentioned this pull request Nov 6, 2020

Invalid extraction text issue (BACKPORT to 10.2.x) #39589

Closed

mhevery closed this in 3f4fe45 Nov 6, 2020

petebacondarwin mentioned this pull request Nov 13, 2020

ng extract-i18n extracts incorrect equiv-text value #39671

Closed

petebacondarwin deleted the invalid-extraction-text-issue-39195 branch November 19, 2020 10:12

petebacondarwin mentioned this pull request Nov 23, 2020

extract-i18n generates wrong interpolation equiv-text in v11 #39654

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 20, 2020

pullapprove bot added the area: core Issues related to the framework runtime label Dec 20, 2020

ngbot bot modified the milestones: needsTriage, Backlog Dec 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(compiler): invalid extraction text#39486

fix(compiler): invalid extraction text#39486
petebacondarwin wants to merge 4 commits intoangular:masterfrom
petebacondarwin:invalid-extraction-text-issue-39195

petebacondarwin commented Oct 29, 2020

Uh oh!

AndrewKushnir left a comment

Uh oh!

Uh oh!

atscott commented Oct 30, 2020

Uh oh!

atscott left a comment

Uh oh!

Uh oh!

Uh oh!

petebacondarwin commented Oct 31, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AndrewKushnir commented Nov 1, 2020

Uh oh!

AndrewKushnir commented Nov 2, 2020

Uh oh!

angular-automatic-lock-bot bot commented Dec 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

petebacondarwin commented Oct 29, 2020

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

atscott commented Oct 30, 2020

Uh oh!

atscott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

petebacondarwin commented Oct 31, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AndrewKushnir commented Nov 1, 2020

Uh oh!

AndrewKushnir commented Nov 2, 2020

Uh oh!

angular-automatic-lock-bot bot commented Dec 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants