8000 fix(compiler): invalid extraction text by petebacondarwin · Pull Request #39486 · angular/angular · GitHub
[go: up one dir, main page]
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): invalid extraction text #39486

Closed
wants to merge 4 commits into from
Closed

fix(compiler): invalid extraction text #39486

wants to merge 4 commits into from

Conversation

petebacondarwin
Copy link
Contributor

Fixes #39195

@petebacondarwin 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 ngbot bot added this to the needsTriage milestone Oct 29, 2020
@google-cla google-cla bot added the cla: yes label Oct 29, 2020
@pullapprove pullapprove bot requested a review from alxhub October 29, 2020 09:50
@petebacondarwin petebacondarwin changed the title fix invalid extraction text fix(compiler): invalid extraction text Oct 29, 2020
@petebacondarwin petebacondarwin added the area: compiler Issues related to `ngc`, Angular's template compiler label Oct 29, 2020
AndrewKushnir
AndrewKushnir approved these changes Oct 30, 2020
View reviewed changes
Copy link
Contributor
@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

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.

@AndrewKushnir AndrewKushnir requested a review from atscott October 30, 2020 23:14
@atscott
Copy link
Contributor
atscott commented Oct 30, 2020

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

@atscott atscott requested a review from JoostK October 30, 2020 23:19
atscott
atscott approved these changes Oct 30, 2020
View reviewed changes
Copy link
Contributor
@atscott atscott left a comment

Choose a reason for hiding this comment

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

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

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

Thanks for the reviews @AndrewKushnir and @atscott !

@petebacondarwin petebacondarwin added the action: presubmit The PR is in need of a google3 presubmit label Oct 31, 2020
@AndrewKushnir
Copy link
Contributor

Presubmit.

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

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

@petebacondarwin petebacondarwin added the action: merge The PR is ready for merge by the caretaker label Nov 3, 2020
@petebacondarwin petebacondarwin removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Nov 3, 2020
@josephperrott josephperrott force-pushed the invalid-extraction-text-issue-39195 branch from 9260b6c to 04a6d42 Compare November 4, 2020 21:59
mhevery pushed a commit that referenced this pull request Nov 5, 2020
@petebacondarwin @mhevery
fix(compiler): ensure that i18n message-parts have the correct source…
63f9e16 
…-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
mhevery pushed a commit that referenced this pull request Nov 5, 2020
@petebacondarwin @mhevery
refactor(compiler): store the fullStart location on `ParseSourceSpa…
92f833b 
…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
mhevery pushed a commit that referenced this pull request Nov 5, 2020
@petebacondarwin @mhevery
refactor(compiler): capture fullStart locations when tokenizing (#3…
94d0013 
…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
mhevery pushed a commit that referenced this pull request Nov 5, 2020
@petebacondarwin @mhevery
fix(compiler): skipping leading whitespace should not break placehold…
b8e9b3d 
…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
@petebacondarwin
fix(compiler): ensure that i18n message-parts have the correct source…
2b89097 
…-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.
@petebacondarwin
refactor(compiler): store the fullStart location on ParseSourceSpans
be4892f 
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.
@petebacondarwin
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
fix(compiler): skipping leading whitespace should not break placehold…
18d2395 
…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
10000
@petebacondarwin petebacondarwin force-pushed the invalid-extraction-text-issue-39195 branch from 04a6d42 to 18d2395 Compare November 6, 2020 16:36
@petebacondarwin petebacondarwin mentioned this pull request Nov 6, 2020
Closed
@mhevery mhevery closed this in 3f4fe45 Nov 6, 2020
mhevery pushed a commit that referenced this pull request Nov 6, 2020
@petebacondarwin @mhevery
refactor(compiler): store the fullStart location on `ParseSourceSpa…
8d90c1a 
…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
mhevery pushed a commit that referenced this pull request Nov 6, 2020
@petebacondarwin @mhevery
refactor(compiler): capture fullStart locations when tokenizing (#3…
43d8e9a 
…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
mhevery pushed a commit that referenced this pull request Nov 6, 2020
@petebacondarwin @mhevery
fix(compiler): skipping leading whitespace should not break placehold…
1f95618 
…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
@petebacondarwin petebacondarwin mentioned this pull request Nov 13, 2020
Closed
@petebacondarwin petebacondarwin deleted the invalid-extraction-text-issue-39195 branch November 19, 2020 10:12
@petebacondarwin petebacondarwin mentioned this pull request Nov 23, 2020
Closed
@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 Dec 20, 2020
@pullapprove pullapprove bot added the area: core Issues related to the framework runtime label Dec 20, 2020
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Dec 20, 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

@atscott atscott atscott approved these changes

@alxhub alxhub alxhub approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler area: core Issues related to the framework runtime area: i18n Issues related to localization and internationalization cla: yes target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

i18n: localize message extraction producing incorrect equiv-text values
5 participants
@petebacondarwin @atscott @AndrewKushnir @JoostK @alxhub
0