Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 9c8a1f8

Browse filesBrowse files
petebacondarwinthePunderWoman
petebacondarwin
authored and
thePunderWoman
committed
fix(compiler): include leading whitespace in source-spans of i18n messages (#43132)
Previously, the way templates were tokenized meant that we lost information about the location of interpolations if the template contained encoded HTML entities. This meant that the mapping back to the source interpolated strings could be offset incorrectly. Also, the source-span assigned to an i18n message did not include leading whitespace. This confused the output source-mappings so that the first text nodes of the message stopped at the first non-whitespace character. This commit makes use of the previous refactorings, where more fine grain information was provided in text tokens, to enable the parser to identify the location of the interpolations in the original source more accurately. Fixes #41034 PR Close #43132
1 parent 6c54a0a commit 9c8a1f8
Copy full SHA for 9c8a1f8

File tree

Expand file treeCollapse file tree

9 files changed

+112
-96
lines changed
Filter options
Expand file treeCollapse file tree

9 files changed

+112
-96
lines changed

‎packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/localize_legacy_message_ids/GOLDEN_PARTIAL.js

Copy file name to clipboardExpand all lines: packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/localize_legacy_message_ids/GOLDEN_PARTIAL.js
+16-2Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,28 @@ export class MyComponent {
77
}
88
MyComponent.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyComponent, deps: [], target: i0.ɵɵFactoryTarget.Component });
99
MyComponent.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", type: MyComponent, selector: "my-component", ngImport: i0, template: `
10-
<div i18n>Some Message</div>
10+
<div i18n-title title="Some &amp; attribute"></div>
11+
<div i18n>Some &amp; message</div>
12+
<div i18n-title title="Some &amp; {{'interpolated'}} attribute"></div>
13+
<div i18n>Some &amp; {{'interpolated' }} message</div>
14+
<div i18n>&amp;</div>
15+
<div i18n>&amp;&quot;</div>
16+
<div i18n-title title="&quot;"></div>
17+
<div i18n-title title="&quot;&quot;"></div>
1118
`, isInline: true });
1219
i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyComponent, decorators: [{
1320
type: Component,
1421
args: [{
1522
selector: 'my-component',
1623
template: `
17-
<div i18n>Some Message</div>
24+
<div i18n-title title="Some &amp; attribute"></div>
25+
<div i18n>Some &amp; message</div>
26+
<div i18n-title title="Some &amp; {{'interpolated'}} attribute"></div>
27+
<div i18n>Some &amp; {{'interpolated' }} message</div>
28+
<div i18n>&amp;</div>
29+
<div i18n>&amp;&quot;</div>
30+
<div i18n-title title="&quot;"></div>
31+
<div i18n-title title="&quot;&quot;"></div>
1832
`
1933
}]
2034
}] });

‎packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/localize_legacy_message_ids/legacy_enabled.js

Copy file name to clipboard
+11-5Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,11 @@
1-
let $I18N_0$;
2-
if (typeof ngI18nClosureMode !== "undefined" && ngI18nClosureMode) { … }
3-
else {
4-
$I18N_0$ = $localize `:␟ec93160d6d6a8822214060dd7938bf821c22b226␟6795333002533525253:Some Message`;
5-
}
1+
$localize `:␟82ec661067f503a3357ecc159b2128325e9208cd␟2908931752694090721:Some & attribute`;
2+
…
3+
$localize `:␟10adaf0ad7b8ba40200cd3c0e7c8d0f13280d522␟4700340487900776701:Some & message`;
4+
…
5+
$localize `:␟57ebd20267116c04cc1dbd7be0b73bf56484f45d␟2334195497629636162:Some & ${"\uFFFD0\uFFFD"}:INTERPOLATION: attribute`;
6+
…
7+
$localize `:␟28d558ca32556f1da67a333e3dada321a97212cd␟3204054277547499090:Some & ${"\uFFFD0\uFFFD"}:INTERPOLATION: message`;
8+
…
9+
$localize `:␟0b3dff7b9382b6217ac97c99f9b04df04381bfdd␟2406634758623728945:&`;
10+
…
11+
$localize `:␟25b7cbf210e59a931423097cb7f2e1b72991a687␟4156372478368653226:&"`;

‎packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/localize_legacy_message_ids/legacy_enabled.ts

Copy file name to clipboardExpand all lines: packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/localize_legacy_message_ids/legacy_enabled.ts
+9-2Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,19 @@ import {Component, NgModule} from '@angular/core';
33
@Component({
44
selector: 'my-component',
55
template: `
6-
<div i18n>Some Message</div>
6+
<div i18n-title title="Some &amp; attribute"></div>
7+
<div i18n>Some &amp; message</div>
8+
<div i18n-title title="Some &amp; {{'interpolated'}} attribute"></div>
9+
<div i18n>Some &amp; {{'interpolated' }} message</div>
10+
<div i18n>&amp;</div>
11+
<div i18n>&amp;&quot;</div>
12+
<div i18n-title title="&quot;"></div>
13+
<div i18n-title title="&quot;&quot;"></div>
714
`
815
})
916
export class MyComponent {
1017
}
1118

1219
@NgModule({declarations: [MyComponent]})
1320
export class MyModule {
14-
}
21+
}

‎packages/compiler-cli/test/compliance/test_cases/source_mapping/inline_templates/i18n_message_element_whitespace.js

Copy file name to clipboardExpand all lines: packages/compiler-cli/test/compliance/test_cases/source_mapping/inline_templates/i18n_message_element_whitespace.js
+5-5Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,19 @@
1-
` pre-p ${ // SOURCE: "/i18n_message_element_whitespace.ts" "pre-p\\n "
1+
` pre-p ${ // SOURCE: "/i18n_message_element_whitespace.ts" "\\n pre-p\\n "
22
…
3-
"\uFFFD#2\uFFFD" // SOURCE: "/i18n_message_element_whitespace.ts" "<p>\\n "
3+
"\uFFFD#2\uFFFD" // SOURCE: "/i18n_message_element_whitespace.ts" "<p>"
44
…
5-
}:START_PARAGRAPH: in-p ${ // SOURCE: "/i18n_message_element_whitespace.ts" "in-p\\n "
5+
}:START_PARAGRAPH: in-p ${ // SOURCE: "/i18n_message_element_whitespace.ts" "\\n in-p\\n "
66
…
77
"\uFFFD/#2\uFFFD" // SOURCE: "/i18n_message_element_whitespace.ts" "</p>"
88
…
9-
}:CLOSE_PARAGRAPH: post-p\n` // SOURCE: "/i18n_message_element_whitespace.ts" "post-p\\n"
9+
}:CLOSE_PARAGRAPH: post-p\n` // SOURCE: "/i18n_message_element_whitespace.ts" "\\n post-p\\n"
1010
…
1111

1212
i0.ɵɵelementStart(0, "div") // SOURCE: "/i18n_message_element_whitespace.ts" "<div i18n>"
1313
…
1414
i0.ɵɵi18nStart(1, 0) // SOURCE: "/i18n_message_element_whitespace.ts" "<div i18n>"
1515
…
16-
i0.ɵɵelement(2, "p") // SOURCE: "/i18n_message_element_whitespace.ts" "<p>\\n "
16+
i0.ɵɵelement(2, "p") // SOURCE: "/i18n_message_element_whitespace.ts" "<p>"
1717
…
1818
i0.ɵɵi18nEnd() // SOURCE: "/i18n_message_element_whitespace.ts" "</div>"
1919
…

‎packages/compiler-cli/test/compliance/test_cases/source_mapping/inline_templates/i18n_message_element_whitespace_partial.js

Copy file name to clipboardExpand all lines: packages/compiler-cli/test/compliance/test_cases/source_mapping/inline_templates/i18n_message_element_whitespace_partial.js
+8-8Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,19 @@
1-
$localize` pre-p ${ // SOURCE: "/i18n_message_element_whitespace.ts" "pre-p\\n "
1+
$localize` pre-p ${ // SOURCE: "/i18n_message_element_whitespace.ts" "\\n pre-p\\n "
22
…
3-
"\uFFFD#2\uFFFD" // SOURCE: "/i18n_message_element_whitespace.ts" "<p>\\n "
3+
"\uFFFD#2\uFFFD" // SOURCE: "/i18n_message_element_whitespace.ts" "<p>"
44
…
5-
}:START_PARAGRAPH: in-p ${ // SOURCE: "/i18n_message_element_whitespace.ts" "in-p\\n "
5+
}:START_PARAGRAPH: in-p ${ // SOURCE: "/i18n_message_element_whitespace.ts" "\\n in-p\\n "
66
…
7-
"\uFFFD/#2\uFFFD" // SOURCE: "/i18n_message_element_whitespace.ts" "</p>\\n "
7+
"\uFFFD/#2\uFFFD" // SOURCE: "/i18n_message_element_whitespace.ts" "</p>"
88
…
9-
}:CLOSE_PARAGRAPH: post-p\n` // SOURCE: "/i18n_message_element_whitespace.ts" "post-p\\n"
9+
}:CLOSE_PARAGRAPH: post-p\n` // SOURCE: "/i18n_message_element_whitespace.ts" "\\n post-p\\n"
1010
…
1111

12-
.ɵɵelementStart(0, "div") // SOURCE: "/i18n_message_element_whitespace.ts" "<div i18n>\\n "
12+
.ɵɵelementStart(0, "div") // SOURCE: "/i18n_message_element_whitespace.ts" "<div i18n>"
1313
…
14-
.ɵɵi18nStart(1, 0) // SOURCE: "/i18n_message_element_whitespace.ts" "<div i18n>\\n "
14+
.ɵɵi18nStart(1, 0) // SOURCE: "/i18n_message_element_whitespace.ts" "<div i18n>"
1515
…
16-
.ɵɵelement(2, "p") // SOURCE: "/i18n_message_element_whitespace.ts" "<p>\\n "
16+
.ɵɵelement(2, "p") // SOURCE: "/i18n_message_element_whitespace.ts" "<p>"
1717
…
1818
.ɵɵi18nEnd() // SOURCE: "/i18n_message_element_whitespace.ts" "</div>'"
1919
…

‎packages/compiler-cli/test/ngtsc/template_mapping_spec.ts

Copy file name to clipboardExpand all lines: packages/compiler-cli/test/ngtsc/template_mapping_spec.ts
+5-5Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -464,27 +464,27 @@ runInEachFileSystem((os) => {
464464
// $localize expressions
465465
expectMapping(mappings, {
466466
sourceUrl: '../test.ts',
467-
source: 'pre-p\n ',
467+
source: '\n pre-p\n ',
468468
generated: '` pre-p ${',
469469
});
470470
expectMapping(mappings, {
471471
sourceUrl: '../test.ts',
472-
source: '<p>\n ',
472+
source: '<p>',
473473
generated: '"\\uFFFD#2\\uFFFD"',
474474
});
475475
expectMapping(mappings, {
476476
sourceUrl: '../test.ts',
477-
source: 'in-p\n ',
477+
source: '\n in-p\n ',
478478
generated: '}:START_PARAGRAPH: in-p ${',
479479
});
480480
expectMapping(mappings, {
481481
sourceUrl: '../test.ts',
482-
source: '</p>\n ',
482+
source: '</p>',
483483
generated: '"\\uFFFD/#2\\uFFFD"',
484484
});
485485
expectMapping(mappings, {
486486
sourceUrl: '../test.ts',
487-
source: 'post-p\n',
487+
source: '\n post-p\n',
488488
generated: '}:CLOSE_PARAGRAPH: post-p\n`',
489489
});
490490
// ivy instructions

‎packages/compiler/src/i18n/i18n_parser.ts

Copy file name to clipboardExpand all lines: packages/compiler/src/i18n/i18n_parser.ts
+52-66Lines changed: 52 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,11 @@
77
*/
88

99
import {Lexer as ExpressionLexer} from '../expression_parser/lexer';
10-
import {InterpolationPiece, Parser as ExpressionParser} from '../expression_parser/parser';
10+
import {Parser as ExpressionParser} from '../expression_parser/parser';
1111
import * as html from '../ml_parser/ast';
1212
import {getHtmlTagDefinition} from '../ml_parser/html_tags';
1313
import {InterpolationConfig} from '../ml_parser/interpolation_config';
14+
import {Token, TokenType} from '../ml_parser/lexer';
1415
import {ParseSourceSpan} from '../parse_util';
1516

1617
import * as i18n from './i18n_ast';
@@ -105,13 +106,18 @@ class _I18nVisitor implements html.Visitor {
105106
}
106107

107108
visitAttribute(attribute: html.Attribute, context: I18nMessageVisitorContext): i18n.Node {
108-
const node = this._visitTextWithInterpolation(
109-
attribute.value, attribute.valueSpan || attribute.sourceSpan, context, attribute.i18n);
109+
const node = attribute.valueTokens === undefined || attribute.valueTokens.length === 1 ?
110+
new i18n.Text(attribute.value, attribute.valueSpan || attribute.sourceSpan) :
111+
this._visitTextWithInterpolation(
112+
attribute.valueTokens, attribute.valueSpan || attribute.sourceSpan, context,
113+
attribute.i18n);
110114
return context.visitNodeFn(attribute, node);
111115
}
112116

113117
visitText(text: html.Text, context: I18nMessageVisitorContext): i18n.Node {
114-
const node = this._visitTextWithInterpolation(text.value, text.sourceSpan, context, text.i18n);
118+
const node = text.tokens.length === 1 ?
119+
new i18n.Text(text.value, text.sourceSpan) :
120+
this._visitTextWithInterpolation(text.tokens, text.sourceSpan, context, text.i18n);
115121
return context.visitNodeFn(text, node);
116122
}
117123

@@ -165,66 +171,54 @@ class _I18nVisitor implements html.Visitor {
165171
* @param previousI18n Any i18n metadata associated with this `text` from a previous pass.
166172
*/
167173
private _visitTextWithInterpolation(
168-
text: string, sourceSpan: ParseSourceSpan, context: I18nMessageVisitorContext,
174+
tokens: Token[], sourceSpan: ParseSourceSpan, context: I18nMessageVisitorContext,
169175
previousI18n: i18n.I18nMeta|undefined): i18n.Node {
170-
const {strings, expressions} = this._expressionParser.splitInterpolation(
171-
text, sourceSpan.start.toString(), this._interpolationConfig);
172-
173-
// No expressions, return a single text.
174-
if (expressions.length === 0) {
175-
return new i18n.Text(text, sourceSpan);
176-
}
177-
178176
// Return a sequence of `Text` and `Placeholder` nodes grouped in a `Container`.
179177
const nodes: i18n.Node[] = [];
180-
for (let i = 0; i < strings.length - 1; i++) {
181-
this._addText(nodes, strings[i], sourceSpan);
182-
this._addPlaceholder(nodes, context, expressions[i], sourceSpan);
178+
// We will only create a container if there are actually interpolations,
179+
// so this flag tracks that.
180+
let hasInterpolation = false;
181+
for (const token of tokens) {
182+
switch (token.type) {
183+
case TokenType.INTERPOLATION:
184+
case TokenType.ATTR_VALUE_INTERPOLATION:
185+
hasInterpolation = true;
186+
const expression = token.parts[1];
187+
const baseName = extractPlaceholderName(expression) || 'INTERPOLATION';
188+
const phName = context.placeholderRegistry.getPlaceholderName(baseName, expression);
189+
context.placeholderToContent[phName] = {
190+
text: token.parts.join(''),
191+
sourceSpan: token.sourceSpan
192+
};
193+
nodes.push(new i18n.Placeholder(expression, phName, token.sourceSpan));
194+
break;
195+
default:
196+
if (token.parts[0].length > 0) {
197+
// This token is text or an encoded entity.
198+
// If it is following on from a previous text node then merge it into that node
199+
// Otherwise, if it is following an interpolation, then add a new node.
200+
const previous = nodes[nodes.length - 1];
201+
if (previous instanceof i18n.Text) {
202+
previous.value += token.parts[0];
203+
previous.sourceSpan = new ParseSourceSpan(
204+
previous.sourceSpan.start, token.sourceSpan.end, previous.sourceSpan.fullStart,
205+
previous.sourceSpan.details);
206+
} else {
207+
nodes.push(new i18n.Text(token.parts[0], token.sourceSpan));
208+
}
209+
}
210+
break;
211+
}
183212
}
184-
// The last index contains no expression
185-
this._addText(nodes, strings[strings.length - 1], sourceSpan);
186-
187-
// Whitespace removal may have invalidated the interpolation source-spans.
188-
reusePreviousSourceSpans(nodes, previousI18n);
189-
190-
return new i18n.Container(nodes, sourceSpan);
191-
}
192213

193-
/**
194-
* Create a new `Text` node from the `textPiece` and add it to the `nodes` collection.
195-
*
196-
* @param nodes The nodes to which the created `Text` node should be added.
197-
* @param textPiece The text and relative span information for this `Text` node.
198-
* @param interpolationSpan The span of the whole interpolated text.
199-
*/
200-
private _addText(
201-
nodes: i18n.Node[], textPiece: InterpolationPiece, interpolationSpan: ParseSourceSpan): void {
202-
if (textPiece.text.length > 0) {
203-
// No need to add empty strings
204-
const stringSpan = getOffsetSourceSpan(interpolationSpan, textPiece);
205-
nodes.push(new i18n.Text(textPiece.text, stringSpan));
214+
if (hasInterpolation) {
215+
// Whitespace removal may have invalidated the interpolation source-spans.
216+
reusePreviousSourceSpans(nodes, previousI18n);
217+
return new i18n.Container(nodes, sourceSpan);
218+
} else {
219+
return nodes[0];
206220
}
207221
}
208-
209-
/**
210-
* Create a new `Placeholder` node from the `expression` and add it to the `nodes` collection.
211-
*
212-
* @param nodes The nodes to which the created `Text` node should be added.
213-
* @param context The current context of the visitor, used to compute and store placeholders.
214-
* @param expression The expression text and relative span information for this `Placeholder`
215-
* node.
216-
* @param interpolationSpan The span of the whole interpolated text.
217-
*/
218-
private _addPlaceholder(
219-
nodes: i18n.Node[], context: I18nMessageVisitorContext, expression: InterpolationPiece,
220-
interpolationSpan: ParseSourceSpan): void {
221-
const sourceSpan = getOffsetSourceSpan(interpolationSpan, expression);
222-
const baseName = extractPlaceholderName(expression.text) || 'INTERPOLATION';
223-
const phName = context.placeholderRegistry.getPlaceholderName(baseName, expression.text);
224-
const text = this._interpolationConfig.start + expression.text + this._interpolationConfig.end;
225-
context.placeholderToContent[phName] = {text, sourceSpan};
226-
nodes.push(new i18n.Placeholder(expression.text, phName, sourceSpan));
227-
}
228222
}
229223

230224
/**
@@ -247,7 +241,7 @@ function reusePreviousSourceSpans(nodes: i18n.Node[], previousI18n: i18n.I18nMet
247241

248242
if (previousI18n instanceof i18n.Container) {
249243
// The `previousI18n` is a `Container`, which means that this is a second i18n extraction pass
250-
// after whitespace has been removed from the AST ndoes.
244+
// after whitespace has been removed from the AST nodes.
251245
assertEquivalentNodes(previousI18n.children, nodes);
252246

253247
// Reuse the source-spans from the first pass.
@@ -282,14 +276,6 @@ function assertEquivalentNodes(previousNodes: i18n.Node[], nodes: i18n.Node[]):
282276
}
283277
}
284278

285-
/**
286-
* Create a new `ParseSourceSpan` from the `sourceSpan`, offset by the `start` and `end` values.
287-
*/
288-
function getOffsetSourceSpan(
289-
sourceSpan: ParseSourceSpan, {start, end}: InterpolationPiece): ParseSourceSpan {
290-
return new ParseSourceSpan(sourceSpan.fullStart.moveBy(start), sourceSpan.fullStart.moveBy(end));
291-
}
292-
293279
const _CUSTOM_PH_EXP =
294280
/\/\/[\s\S]*i18n[\s\S]*\([\s\S]*ph[\s\S]*=[\s\S]*("|')([\s\S]*?)\1[\s\S]*\)/g;
295281

‎packages/compiler/src/render3/view/i18n/localize_utils.ts

Copy file name to clipboardExpand all lines: packages/compiler/src/render3/view/i18n/localize_utils.ts
+5-2Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,10 @@ class LocalizeSerializerVisitor implements i18n.Visitor {
3535
// Two literal pieces in a row means that there was some comment node in-between.
3636
context[context.length - 1].text += text.value;
3737
} else {
38-
context.push(new o.LiteralPiece(text.value, text.sourceSpan));
38+
const sourceSpan = new ParseSourceSpan(
39+
text.sourceSpan.fullStart, text.sourceSpan.end, text.sourceSpan.fullStart,
40+
text.sourceSpan.details);
41+
context.push(new o.LiteralPiece(text.value, sourceSpan));
3942
}
4043
}
4144

@@ -90,7 +93,7 @@ function getSourceSpan(message: i18n.Message): ParseSourceSpan {
9093
const startNode = message.nodes[0];
9194
const endNode = message.nodes[message.nodes.length - 1];
9295
return new ParseSourceSpan(
93-
startNode.sourceSpan.start, endNode.sourceSpan.end, startNode.sourceSpan.fullStart,
96+
startNode.sourceSpan.fullStart, endNode.sourceSpan.end, startNode.sourceSpan.fullStart,
9497
startNode.sourceSpan.details);
9598
}
9699

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

Copy file name to clipboardExpand all lines: packages/compiler/test/render3/view/i18n_spec.ts
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -478,7 +478,7 @@ describe('serializeI18nMessageForLocalize', () => {
478478
expect(messageParts[3].text).toEqual('');
479479
expect(messageParts[3].sourceSpan.toString()).toEqual('');
480480
expect(messageParts[4].text).toEqual(' D');
481-
expect(messageParts[4].sourceSpan.toString()).toEqual('D');
481+
expect(messageParts[4].sourceSpan.toString()).toEqual(' D');
482482

483483
expect(placeHolders[0].text).toEqual('START_TAG_SPAN');
484484
expect(placeHolders[0].sourceSpan.toString()).toEqual('<span>');

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.