Skip to content

Commit

Permalink
fix(compiler): ensure that placeholders have the correct sourceSpan
Browse files Browse the repository at this point in the history
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
  • Loading branch information
petebacondarwin committed Nov 16, 2020
1 parent 9f20942 commit 73df49d
Show file tree
Hide file tree
Showing 2 changed files with 73 additions and 3 deletions.
30 changes: 30 additions & 0 deletions packages/compiler-cli/test/ngtsc/template_mapping_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,36 @@ runInEachFileSystem((os) => {
});
});

it('should correctly handle collapsed whitespace in placeholder source-mappings', () => {
const mappings = compileAndMap(
`<div i18n title=" pre-title {{title_value}} post-title" i18n-title> pre-body {{body_value}} post-body</div>`);
expect(mappings).toContain({
source: '<div i18n title=" pre-title {{title_value}} post-title" i18n-title> ',
generated: 'i0.ɵɵelementStart(0, "div", 0)',
sourceUrl: '../test.ts',
});
expect(mappings).toContain({
source: '</div>',
generated: 'i0.ɵɵelementEnd()',
sourceUrl: '../test.ts',
});
expect(mappings).toContain({
source: ' pre-body ',
generated: '` pre-body ${',
sourceUrl: '../test.ts',
});
expect(mappings).toContain({
source: '{{body_value}}',
generated: '"\\uFFFD0\\uFFFD"',
sourceUrl: '../test.ts',
});
expect(mappings).toContain({
source: ' post-body',
generated: '}:INTERPOLATION: post-body`',
sourceUrl: '../test.ts',
});
});

it('should create tag (container) placeholder source-mappings', () => {
const mappings = compileAndMap(`<div i18n>Hello, <b>World</b>!</div>`);
expect(mappings).toContain({
Expand Down
46 changes: 43 additions & 3 deletions packages/compiler/src/i18n/i18n_parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,12 +105,13 @@ class _I18nVisitor implements html.Visitor {
}

visitAttribute(attribute: html.Attribute, context: I18nMessageVisitorContext): i18n.Node {
const node = this._visitTextWithInterpolation(attribute.value, attribute.sourceSpan, context);
const node = this._visitTextWithInterpolation(
attribute.value, attribute.valueSpan || attribute.sourceSpan, context, attribute.i18n);
return context.visitNodeFn(attribute, node);
}

visitText(text: html.Text, context: I18nMessageVisitorContext): i18n.Node {
const node = this._visitTextWithInterpolation(text.value, text.sourceSpan, context);
const node = this._visitTextWithInterpolation(text.value, text.sourceSpan, context, text.i18n);
return context.visitNodeFn(text, node);
}

Expand Down Expand Up @@ -156,7 +157,8 @@ class _I18nVisitor implements html.Visitor {
}

private _visitTextWithInterpolation(
text: string, sourceSpan: ParseSourceSpan, context: I18nMessageVisitorContext): i18n.Node {
text: string, sourceSpan: ParseSourceSpan, context: I18nMessageVisitorContext,
previousI18n: i18n.I18nMeta|undefined): i18n.Node {
const splitInterpolation = this._expressionParser.splitInterpolation(
text, sourceSpan.start.toString(), this._interpolationConfig);

Expand Down Expand Up @@ -197,10 +199,48 @@ class _I18nVisitor implements html.Visitor {
getOffsetSourceSpan(sourceSpan, splitInterpolation.stringSpans[lastStringIdx]);
nodes.push(new i18n.Text(splitInterpolation.strings[lastStringIdx], stringSpan));
}

if (previousI18n instanceof i18n.Message) {
// The `previousI18n` is an i18n `Message`, so we are processing an `Attribute` with i18n
// metadata. The `Message` should consist only of a single `Container` that contains the
// parts (`Text` and `Placeholder`) to process.
assertSingleContainerMessage(previousI18n);
previousI18n = previousI18n.nodes[0];
}

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

// Reuse the source-spans from the first pass.
for (let i = 0; i < nodes.length; i++) {
nodes[i].sourceSpan = previousI18n.children[i].sourceSpan;
}
}

return container;
}
}

function assertSingleContainerMessage(message: i18n.Message): void {
const nodes = message.nodes;
if (nodes.length !== 1 || !(nodes[0] instanceof i18n.Container)) {
throw new Error(
'Unexpected previous i18n message - expected it to consist of only a single `Container` node.');
}
}

function assertEquivalentNodes(previousNodes: i18n.Node[], nodes: i18n.Node[]): void {
if (previousNodes.length !== nodes.length) {
throw new Error('The number of i18n message children changed between first and second pass.');
}
if (previousNodes.some((node, i) => nodes[i].constructor !== node.constructor)) {
throw new Error(
'The types of the i18n message children changed between first and second pass.');
}
}

function getOffsetSourceSpan(
sourceSpan: ParseSourceSpan, {start, end}: {start: number, end: number}): ParseSourceSpan {
return new ParseSourceSpan(sourceSpan.fullStart.moveBy(start), sourceSpan.fullStart.moveBy(end));
Expand Down

0 comments on commit 73df49d

Please sign in to comment.