mirror of
https://github.com/angular/angular
synced 2026-05-24 09:28:37 +00:00
When disabling `i18nPreserveSignificantWhitespaceForLegacyExtraction` I was looking at a test case with ICU messages containing leading and trailing whitespace:
```angular
<div i18n>
{apples, plural, =other {I have many apples.}}
</div>
```
This would historically generate two messages:
```javascript
const MSG_TMP = goog.getMsg('{apples, plural, =other {I have many apples.}}');
const MSG_FOO = goog.getMsg(' {$ICU} ', { 'ICU': MSG_TMP });
```
But I found that I was getting just one message:
```javascript
const MSG_TMP = goog.getMsg(' {apples, plural, =other {I have many apples.}} ');
```
This is arguably an improvement, but changed the messages and message IDs, which isn't desirable with this option. I eventually traced this back to the `isIcu` initialization in [`i18n_parser.ts`](/packages/compiler/src/i18n/i18n_parser.ts):
```typescript
const context: I18nMessageVisitorContext = {
isIcu: nodes.length == 1 && nodes[0] instanceof html.Expansion,
// ...
};
```
[`_I18nVisitor.prototype.visitExpansion`](/packages/compiler/src/i18n/i18n_parser.ts) uses this to decide whether or not to generate a sub-message for a given ICU expansion:
```typescript
if (context.isIcu || context.icuDepth > 0) {
// Returns an ICU node when:
// - the message (vs a part of the message) is an ICU message, or
// - the ICU message is nested.
const expPh = context.placeholderRegistry.getUniquePlaceholder(`VAR_${icu.type}`);
i18nIcu.expressionPlaceholder = expPh;
context.placeholderToContent[expPh] = {
text: icu.switchValue,
sourceSpan: icu.switchValueSourceSpan,
};
return context.visitNodeFn(icu, i18nIcu);
}
// Else returns a placeholder
// ICU placeholders should not be replaced with their original content but with the their
// translations.
// TODO(vicb): add a html.Node -> i18n.Message cache to avoid having to re-create the msg
const phName = context.placeholderRegistry.getPlaceholderName('ICU', icu.sourceSpan.toString());
context.placeholderToMessage[phName] = this.toI18nMessage([icu], '', '', '', undefined);
const node = new i18n.IcuPlaceholder(i18nIcu, phName, icu.sourceSpan);
return context.visitNodeFn(icu, node);
```
Note that `isIcu` is the key condition between these two cases and depends on whether or not the ICU expansion has any siblings. The introduction of `WhitespaceVisitor` to `I18nMetaVisitor` trims insignificant whitespace, including empty text nodes not adjacent to an ICU expansion (from [`WhitespaceVisitor.prototype.visitText`](/packages/compiler/src/ml_parser/html_whitespaces.ts)):
```typescript
const isNotBlank = text.value.match(NO_WS_REGEXP);
const hasExpansionSibling =
context && (context.prev instanceof html.Expansion || context.next instanceof html.Expansion);
if (isNotBlank || hasExpansionSibling) {
// Transform node by trimming it...
return trimmedNode;
}
return null; // Drop node which is empty and has no ICU expansion sibling.
```
`hasExpansionSibling` was intended to retain empty text nodes leading or trailing an ICU expansion, however `context` was `undefined`, so this check failed and the leading / trailing text nodes were dropped. This resulted in trimming the ICU text by dropping the leading / trailing whitespace nodes. Having only a single ICU expansion with no leading / trailing text nodes caused `_I18nVisitor` to initialize `isIcu` incorrectly and caused it to generate one message instead of two.
`WhitespaceVisitor` is supposed to get this context from `visitAllWithSiblings`. So the fix here is to make sure `WhitespaceVisitor` is always visited via this function which provides the required context. I updated all usage sites to make sure this context is use consistently and implemented the `WhitespaceVisitor.prototype.visit` method to throw when the context is missing to make sure we don't encounter a similar mistake in the future.
Unfortunately this broke one compliance test. Specifically the [`icu_logic/icu_only.js`](/home/douglasparker/Source/ng/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_i18n/icu_logic/icu_only.js) test which changed from generating:
```javascript
function MyComponent_Template(rf, ctx) {
if (rf & 1) {
i0.ɵɵi18n(0, 0);
}
// ...
}
```
To now generating:
```javascript
function MyComponent_Template(rf, ctx) {
if (rf & 1) {
i0.ɵɵtext(0, " ");
i0.ɵɵi18n(1, 0);
i0.ɵɵtext(2, "\n");
}
// ...
}
```
This test uses the default value `preserveWhitespaces: false` (`i18nPreserveSignificantWhitespaceForLegacyExtraction` should not affect compiled JS output, we already retain significant whitespace there). So what this indicates to me is that ICU logic is already broken because it's not preserving significant whitespace in this case. My change is probably a bug fix, but one which would affect the compiled runtime, which is not in scope here. The root cause is because using `visitAllWithSiblings` everywhere means the context is retained correctly in this case and the whitespace is leading/trailing an ICU message, therefore it is retained per the logic of `WhitespaceVisitor.prototype.visitText` I mentioned eariler.
To address this, I left one usage of `WhitespaceVisitor` using `html.visitAll` instead of `visitAllWithSiblings` to retain this bug. I has to lossen the assertion I put in `WhitespaceVisitor.prototype.visit` to make this possible, but it should still throw by default when misused, which is the important part.
PR Close #56507
208 lines
6.6 KiB
TypeScript
208 lines
6.6 KiB
TypeScript
/**
|
|
* @license
|
|
* Copyright Google LLC All Rights Reserved.
|
|
*
|
|
* Use of this source code is governed by an MIT-style license that can be
|
|
* found in the LICENSE file at https://angular.io/license
|
|
*/
|
|
|
|
import * as e from '../../../src/expression_parser/ast';
|
|
import {Lexer} from '../../../src/expression_parser/lexer';
|
|
import {Parser} from '../../../src/expression_parser/parser';
|
|
import * as html from '../../../src/ml_parser/ast';
|
|
import {DEFAULT_INTERPOLATION_CONFIG, InterpolationConfig} from '../../../src/ml_parser/defaults';
|
|
import {HtmlParser} from '../../../src/ml_parser/html_parser';
|
|
import {WhitespaceVisitor, visitAllWithSiblings} from '../../../src/ml_parser/html_whitespaces';
|
|
import {ParseTreeResult} from '../../../src/ml_parser/parser';
|
|
import * as a from '../../../src/render3/r3_ast';
|
|
import {htmlAstToRender3Ast, Render3ParseResult} from '../../../src/render3/r3_template_transform';
|
|
import {I18nMetaVisitor} from '../../../src/render3/view/i18n/meta';
|
|
import {LEADING_TRIVIA_CHARS} from '../../../src/render3/view/template';
|
|
import {ElementSchemaRegistry} from '../../../src/schema/element_schema_registry';
|
|
import {BindingParser} from '../../../src/template_parser/binding_parser';
|
|
|
|
class MockSchemaRegistry implements ElementSchemaRegistry {
|
|
constructor(
|
|
public existingProperties: {[key: string]: boolean},
|
|
public attrPropMapping: {[key: string]: string},
|
|
public existingElements: {[key: string]: boolean},
|
|
public invalidProperties: Array<string>,
|
|
public invalidAttributes: Array<string>,
|
|
) {}
|
|
|
|
hasProperty(tagName: string, property: string, schemas: any[]): boolean {
|
|
const value = this.existingProperties[property];
|
|
return value === void 0 ? true : value;
|
|
}
|
|
|
|
hasElement(tagName: string, schemaMetas: any[]): boolean {
|
|
const value = this.existingElements[tagName.toLowerCase()];
|
|
return value === void 0 ? true : value;
|
|
}
|
|
|
|
allKnownElementNames(): string[] {
|
|
return Object.keys(this.existingElements);
|
|
}
|
|
|
|
securityContext(selector: string, property: string, isAttribute: boolean): any {
|
|
return 0;
|
|
}
|
|
|
|
getMappedPropName(attrName: string): string {
|
|
return this.attrPropMapping[attrName] || attrName;
|
|
}
|
|
|
|
getDefaultComponentElementName(): string {
|
|
return 'ng-component';
|
|
}
|
|
|
|
validateProperty(name: string): {error: boolean; msg?: string} {
|
|
if (this.invalidProperties.indexOf(name) > -1) {
|
|
return {error: true, msg: `Binding to property '${name}' is disallowed for security reasons`};
|
|
} else {
|
|
return {error: false};
|
|
}
|
|
}
|
|
|
|
validateAttribute(name: string): {error: boolean; msg?: string} {
|
|
if (this.invalidAttributes.indexOf(name) > -1) {
|
|
return {
|
|
error: true,
|
|
msg: `Binding to attribute '${name}' is disallowed for security reasons`,
|
|
};
|
|
} else {
|
|
return {error: false};
|
|
}
|
|
}
|
|
|
|
normalizeAnimationStyleProperty(propName: string): string {
|
|
return propName;
|
|
}
|
|
normalizeAnimationStyleValue(
|
|
camelCaseProp: string,
|
|
userProvidedProp: string,
|
|
val: string | number,
|
|
): {error: string; value: string} {
|
|
return {error: null!, value: val.toString()};
|
|
}
|
|
}
|
|
|
|
export function findExpression(tmpl: a.Node[], expr: string): e.AST | null {
|
|
const res = tmpl.reduce(
|
|
(found, node) => {
|
|
if (found !== null) {
|
|
return found;
|
|
} else {
|
|
return findExpressionInNode(node, expr);
|
|
}
|
|
},
|
|
null as e.AST | null,
|
|
);
|
|
if (res instanceof e.ASTWithSource) {
|
|
return res.ast;
|
|
}
|
|
return res;
|
|
}
|
|
|
|
function findExpressionInNode(node: a.Node, expr: string): e.AST | null {
|
|
if (node instanceof a.Element || node instanceof a.Template) {
|
|
return findExpression([...node.inputs, ...node.outputs, ...node.children], expr);
|
|
} else if (node instanceof a.BoundAttribute || node instanceof a.BoundText) {
|
|
const ts = toStringExpression(node.value);
|
|
return toStringExpression(node.value) === expr ? node.value : null;
|
|
} else if (node instanceof a.BoundEvent) {
|
|
return toStringExpression(node.handler) === expr ? node.handler : null;
|
|
} else {
|
|
return null;
|
|
}
|
|
}
|
|
|
|
export function toStringExpression(expr: e.AST): string {
|
|
while (expr instanceof e.ASTWithSource) {
|
|
expr = expr.ast;
|
|
}
|
|
if (expr instanceof e.PropertyRead) {
|
|
if (expr.receiver instanceof e.ImplicitReceiver) {
|
|
return expr.name;
|
|
} else {
|
|
return `${toStringExpression(expr.receiver)}.${expr.name}`;
|
|
}
|
|
} else if (expr instanceof e.ImplicitReceiver) {
|
|
return '';
|
|
} else if (expr instanceof e.Interpolation) {
|
|
let str = '{{';
|
|
for (let i = 0; i < expr.expressions.length; i++) {
|
|
str += expr.strings[i] + toStringExpression(expr.expressions[i]);
|
|
}
|
|
str += expr.strings[expr.strings.length - 1] + '}}';
|
|
return str;
|
|
} else {
|
|
throw new Error(`Unsupported type: ${(expr as any).constructor.name}`);
|
|
}
|
|
}
|
|
|
|
// Parse an html string to IVY specific info
|
|
export function parseR3(
|
|
input: string,
|
|
options: {
|
|
preserveWhitespaces?: boolean;
|
|
leadingTriviaChars?: string[];
|
|
ignoreError?: boolean;
|
|
} = {},
|
|
): Render3ParseResult {
|
|
const htmlParser = new HtmlParser();
|
|
const parseResult = htmlParser.parse(input, 'path:://to/template', {
|
|
tokenizeExpansionForms: true,
|
|
leadingTriviaChars: options.leadingTriviaChars ?? LEADING_TRIVIA_CHARS,
|
|
});
|
|
|
|
if (parseResult.errors.length > 0 && !options.ignoreError) {
|
|
const msg = parseResult.errors.map((e) => e.toString()).join('\n');
|
|
throw new Error(msg);
|
|
}
|
|
|
|
let htmlNodes = processI18nMeta(parseResult).rootNodes;
|
|
|
|
if (!options.preserveWhitespaces) {
|
|
htmlNodes = visitAllWithSiblings(
|
|
new WhitespaceVisitor(true /* preserveSignificantWhitespace */),
|
|
htmlNodes,
|
|
);
|
|
}
|
|
|
|
const expressionParser = new Parser(new Lexer());
|
|
const schemaRegistry = new MockSchemaRegistry(
|
|
{'invalidProp': false},
|
|
{'mappedAttr': 'mappedProp'},
|
|
{'unknown': false, 'un-known': false},
|
|
['onEvent'],
|
|
['onEvent'],
|
|
);
|
|
const bindingParser = new BindingParser(
|
|
expressionParser,
|
|
DEFAULT_INTERPOLATION_CONFIG,
|
|
schemaRegistry,
|
|
[],
|
|
);
|
|
const r3Result = htmlAstToRender3Ast(htmlNodes, bindingParser, {collectCommentNodes: false});
|
|
|
|
if (r3Result.errors.length > 0 && !options.ignoreError) {
|
|
const msg = r3Result.errors.map((e) => e.toString()).join('\n');
|
|
throw new Error(msg);
|
|
}
|
|
|
|
return r3Result;
|
|
}
|
|
|
|
export function processI18nMeta(
|
|
htmlAstWithErrors: ParseTreeResult,
|
|
interpolationConfig: InterpolationConfig = DEFAULT_INTERPOLATION_CONFIG,
|
|
): ParseTreeResult {
|
|
return new ParseTreeResult(
|
|
html.visitAll(
|
|
new I18nMetaVisitor(interpolationConfig, /* keepI18nAttrs */ false),
|
|
htmlAstWithErrors.rootNodes,
|
|
),
|
|
htmlAstWithErrors.errors,
|
|
);
|
|
}
|