Skip to content

Commit 8ec7156

Browse files
petebacondarwinAndrewKushnir
petebacondarwin
authored and
AndrewKushnir
committed
fix(compiler): ensure that placeholders have the correct sourceSpan (#39717)
When the `preserveWhitespaces` is not true, the template parser will process the parsed AST nodes to remove excess whitespace. Since the generated `goog.getMsg()` statements rely upon the AST nodes after this whitespace is removed, the i18n extraction must make a second pass. Previously this resulted in innacurrate source-spans for the i18n text and placeholder nodes that were extracted in the second pass. This commit fixes this by reusing the source-spans from the first pass when extracting the nodes in the second pass. Fixes #39671 PR Close #39717
1 parent d172882 commit 8ec7156

File tree

2 files changed

+132
-3
lines changed

2 files changed

+132
-3
lines changed

Diff for: ‎packages/compiler-cli/test/ngtsc/template_mapping_spec.ts

+89
Original file line numberDiff line numberDiff line change
@@ -427,6 +427,95 @@ runInEachFileSystem((os) => {
427427
});
428428
});
429429

430+
it('should correctly handle collapsed whitespace in interpolation placeholder source-mappings',
431+
() => {
432+
const mappings = compileAndMap(
433+
`<div i18n title=" pre-title {{title_value}} post-title" i18n-title> pre-body {{body_value}} post-body</div>`);
434+
expectMapping(mappings, {
435+
source: '<div i18n title=" pre-title {{title_value}} post-title" i18n-title> ',
436+
generated: 'i0.ɵɵelementStart(0, "div", 0)',
437+
sourceUrl: '../test.ts',
438+
});
439+
expectMapping(mappings, {
440+
source: '</div>',
441+
generated: 'i0.ɵɵelementEnd()',
442+
sourceUrl: '../test.ts',
443+
});
444+
expectMapping(mappings, {
445+
source: ' pre-body ',
446+
generated: '` pre-body ${',
447+
sourceUrl: '../test.ts',
448+
});
449+
expectMapping(mappings, {
450+
source: '{{body_value}}',
451+
generated: '"\\uFFFD0\\uFFFD"',
452+
sourceUrl: '../test.ts',
453+
});
454+
expectMapping(mappings, {
455+
source: ' post-body',
456+
generated: '}:INTERPOLATION: post-body`',
457+
sourceUrl: '../test.ts',
458+
});
459+
});
460+
461+
it('should correctly handle collapsed whitespace in element placeholder source-mappings',
462+
() => {
463+
const mappings =
464+
compileAndMap(`<div i18n>\n pre-p\n <p>\n in-p\n </p>\n post-p\n</div>`);
465+
// $localize expressions
466+
expectMapping(mappings, {
467+
sourceUrl: '../test.ts',
468+
source: 'pre-p\n ',
469+
generated: '` pre-p ${',
470+
});
471+
expectMapping(mappings, {
472+
sourceUrl: '../test.ts',
473+
source: '<p>\n ',
474+
generated: '"\\uFFFD#2\\uFFFD"',
475+
});
476+
expectMapping(mappings, {
477+
sourceUrl: '../test.ts',
478+
source: 'in-p\n ',
479+
generated: '}:START_PARAGRAPH: in-p ${',
480+
});
481+
expectMapping(mappings, {
482+
sourceUrl: '../test.ts',
483+
source: '</p>\n ',
484+
generated: '"\\uFFFD/#2\\uFFFD"',
485+
});
486+
expectMapping(mappings, {
487+
sourceUrl: '../test.ts',
488+
source: 'post-p\n',
489+
generated: '}:CLOSE_PARAGRAPH: post-p\n`',
490+
});
491+
// ivy instructions
492+
expectMapping(mappings, {
493+
sourceUrl: '../test.ts',
494+
source: '<div i18n>\n ',
495+
generated: 'i0.ɵɵelementStart(0, "div")',
496+
});
497+
expectMapping(mappings, {
498+
sourceUrl: '../test.ts',
499+
source: '<div i18n>\n ',
500+
generated: 'i0.ɵɵi18nStart(1, 0)',
501+
});
502+
expectMapping(mappings, {
503+
sourceUrl: '../test.ts',
504+
source: '<p>\n in-p\n </p>',
505+
generated: 'i0.ɵɵelement(2, "p")',
506+
});
507+
expectMapping(mappings, {
508+
sourceUrl: '../test.ts',
509+
source: '</div>',
510+
generated: 'i0.ɵɵi18nEnd()',
511+
});
512+
expectMapping(mappings, {
513+
sourceUrl: '../test.ts',
514+
source: '</div>',
515+
generated: 'i0.ɵɵelementEnd()',
516+
});
517+
});
518+
430519
it('should create tag (container) placeholder source-mappings', () => {
431520
const mappings = compileAndMap(`<div i18n>Hello, <b>World</b>!</div>`);
432521
expectMapping(mappings, {

Diff for: ‎packages/compiler/src/i18n/i18n_parser.ts

+43-3
Original file line numberDiff line numberDiff line change
@@ -105,12 +105,13 @@ class _I18nVisitor implements html.Visitor {
105105
}
106106

107107
visitAttribute(attribute: html.Attribute, context: I18nMessageVisitorContext): i18n.Node {
108-
const node = this._visitTextWithInterpolation(attribute.value, attribute.sourceSpan, context);
108+
const node = this._visitTextWithInterpolation(
109+
attribute.value, attribute.valueSpan || attribute.sourceSpan, context, attribute.i18n);
109110
return context.visitNodeFn(attribute, node);
110111
}
111112

112113
visitText(text: html.Text, context: I18nMessageVisitorContext): i18n.Node {
113-
const node = this._visitTextWithInterpolation(text.value, text.sourceSpan, context);
114+
const node = this._visitTextWithInterpolation(text.value, text.sourceSpan, context, text.i18n);
114115
return context.visitNodeFn(text, node);
115116
}
116117

@@ -156,7 +157,8 @@ class _I18nVisitor implements html.Visitor {
156157
}
157158

158159
private _visitTextWithInterpolation(
159-
text: string, sourceSpan: ParseSourceSpan, context: I18nMessageVisitorContext): i18n.Node {
160+
text: string, sourceSpan: ParseSourceSpan, context: I18nMessageVisitorContext,
161+
previousI18n: i18n.I18nMeta|undefined): i18n.Node {
160162
const splitInterpolation = this._expressionParser.splitInterpolation(
161163
text, sourceSpan.start.toString(), this._interpolationConfig);
162164

@@ -197,10 +199,48 @@ class _I18nVisitor implements html.Visitor {
197199
getOffsetSourceSpan(sourceSpan, splitInterpolation.stringSpans[lastStringIdx]);
198200
nodes.push(new i18n.Text(splitInterpolation.strings[lastStringIdx], stringSpan));
199201
}
202+
203+
if (previousI18n instanceof i18n.Message) {
204+
// The `previousI18n` is an i18n `Message`, so we are processing an `Attribute` with i18n
205+
// metadata. The `Message` should consist only of a single `Container` that contains the
206+
// parts (`Text` and `Placeholder`) to process.
207+
assertSingleContainerMessage(previousI18n);
208+
previousI18n = previousI18n.nodes[0];
209+
}
210+
211+
if (previousI18n instanceof i18n.Container) {
212+
// The `previousI18n` is a `Container`, which means that this is a second i18n extraction pass
213+
// after whitespace has been removed from the AST ndoes.
214+
assertEquivalentNodes(previousI18n.children, nodes);
215+
216+
// Reuse the source-spans from the first pass.
217+
for (let i = 0; i < nodes.length; i++) {
218+
nodes[i].sourceSpan = previousI18n.children[i].sourceSpan;
219+
}
220+
}
221+
200222
return container;
201223
}
202224
}
203225

226+
function assertSingleContainerMessage(message: i18n.Message): void {
227+
const nodes = message.nodes;
228+
if (nodes.length !== 1 || !(nodes[0] instanceof i18n.Container)) {
229+
throw new Error(
230+
'Unexpected previous i18n message - expected it to consist of only a single `Container` node.');
231+
}
232+
}
233+
234+
function assertEquivalentNodes(previousNodes: i18n.Node[], nodes: i18n.Node[]): void {
235+
if (previousNodes.length !== nodes.length) {
236+
throw new Error('The number of i18n message children changed between first and second pass.');
237+
}
238+
if (previousNodes.some((node, i) => nodes[i].constructor !== node.constructor)) {
239+
throw new Error(
240+
'The types of the i18n message children changed between first and second pass.');
241+
}
242+
}
243+
204244
function getOffsetSourceSpan(
205245
sourceSpan: ParseSourceSpan, {start, end}: {start: number, end: number}): ParseSourceSpan {
206246
return new ParseSourceSpan(sourceSpan.fullStart.moveBy(start), sourceSpan.fullStart.moveBy(end));

0 commit comments

Comments
 (0)