From 06d498c616d571dca886032f4cc6ce11a04f4acd Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Mon, 16 Nov 2020 15:02:38 +0000 Subject: [PATCH] fix(compiler): ensure that placeholders have the correct sourceSpan 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 --- .../test/ngtsc/template_mapping_spec.ts | 30 ++++++++++++ packages/compiler/src/i18n/i18n_parser.ts | 46 +++++++++++++++++-- 2 files changed, 73 insertions(+), 3 deletions(-) diff --git a/packages/compiler-cli/test/ngtsc/template_mapping_spec.ts b/packages/compiler-cli/test/ngtsc/template_mapping_spec.ts index c830f30f8e4828..83cc42b5bbf0b7 100644 --- a/packages/compiler-cli/test/ngtsc/template_mapping_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_mapping_spec.ts @@ -404,6 +404,36 @@ runInEachFileSystem((os) => { }); }); + it('should correctly handle collapsed whitespace in placeholder source-mappings', () => { + const mappings = compileAndMap( + `
pre-body {{body_value}} post-body
`); + expect(mappings).toContain({ + source: '
', + generated: 'i0.ɵɵelementStart(0, "div", 0)', + sourceUrl: '../test.ts', + }); + expect(mappings).toContain({ + source: '
', + 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(`
Hello, World!
`); expect(mappings).toContain({ diff --git a/packages/compiler/src/i18n/i18n_parser.ts b/packages/compiler/src/i18n/i18n_parser.ts index c22941f16f6867..4e85abfc49e68a 100644 --- a/packages/compiler/src/i18n/i18n_parser.ts +++ b/packages/compiler/src/i18n/i18n_parser.ts @@ -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); } @@ -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); @@ -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));