Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ivy): i18n - ensure that colons in i18n metadata are not rendered #33820

Closed

Conversation

@petebacondarwin
Copy link
Member

petebacondarwin commented Nov 14, 2019

The : char is used as a metadata marker in $localize messages.
If this char appears in the metadata it must be escaped, as \:.
Previously, although the : char was being escaped, the TS AST
being generated was not correct and so it was being output double
escaped, which meant that it appeared in the rendered message.

As of TS 3.6.2 the "raw" string can be specified when creating tagged
template AST nodes, so it is possible to correct this.

@petebacondarwin petebacondarwin requested review from angular/fw-compiler as code owners Nov 14, 2019
@ngbot ngbot bot modified the milestone: needsTriage Nov 14, 2019
@googlebot googlebot added the cla: yes label Nov 14, 2019
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-escape-meta-blocks branch from 35672c5 to 33f9a71 Nov 14, 2019
Copy link
Contributor

ocombe left a comment

Looks good, except for debug-test.sh and yarn.lock that shouldn't be part of the PR! :)

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-escape-meta-blocks branch from 33f9a71 to a5df62c Nov 14, 2019
@petebacondarwin

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Nov 14, 2019

Thanks @ocombe - I removed the yarn.lock but I moved the debug-test.sh to its own commit since it is an important fix and might as well go in with this PR.

@ocombe
ocombe approved these changes Nov 14, 2019
Copy link
Contributor

AndrewKushnir left a comment

LGTM 👍

@alxhub
alxhub approved these changes Nov 14, 2019
Copy link
Member

josephperrott left a comment

LGTM

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Nov 15, 2019

New set of g3 presubmits:

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Nov 15, 2019

@petebacondarwin g3 presubmit indicated the following problem:

Error: Debug Failure. False expression: Expected argument 'text' to be the normalized (i.e. 'cooked') version of argument 'rawText'.
    at createTemplateLiteralLikeNode (javascript/node_modules/typescript/stable/lib/typescript.js:64087:22)
    at Object.createNoSubstitutionTemplateLiteral (javascript/node_modules/typescript/stable/lib/typescript.js:64111:20)
    at visitLocalizedString (packages/compiler-cli/src/ngtsc/translator/src/translator.ts:552:19)
...

Even though TS version requirement is satisfied. I sent you a message in Slack with additional info.

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-escape-meta-blocks branch 2 times, most recently from 62dc8f4 to 119ea34 Nov 15, 2019
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-escape-meta-blocks branch from 119ea34 to 6ce72cc Nov 15, 2019
@petebacondarwin

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Nov 15, 2019

From looking at the assertion - TS is re-parsing the raw text as though it was in the original source.

For example if we are creating a simple no-substitution template literal with the text abc they put this back through their normal tokenizer (scanner) wrapped in backticks abc and check that they then get the original abc out.

It is likely that the failing messages contain unescaped backticks or ${ sequences.

For example

ab`c

In this case they would put it through the scanner as

`ab`c`

which would obviously return only ab as the cooked string.

I have added a commit that escapes these sequences.

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-escape-meta-blocks branch from 6ce72cc to d4f2577 Nov 15, 2019
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Nov 15, 2019

Thanks for the fix @petebacondarwin.

I've restarted g3 presubmits with the most up-to-date version of this PR:

@IgorMinar

This comment has been minimized.

Copy link
Member

IgorMinar commented Nov 15, 2019

Still fails with "Debug Failure. False expression: Expected argument 'text' to be the normalized (i.e. 'cooked') version of argument 'rawText'."

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Nov 16, 2019

@petebacondarwin I performed additional investigation and here is the pattern that is causing failures in g3:

<div i18n="Some text \' [BACKUP_MESSAGE_ID: xxx]">Test</div>
@petebacondarwin

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Nov 16, 2019

Thanks @AndrewKushnir - is the \' supposed to be rendered as a ' or as \' in the browser?
I can "fix" this by double escaping any \s that are found in the description, but I am wondering what the actual intent is.

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-escape-meta-blocks branch from 8080c75 to 4c52d4d Nov 16, 2019
@petebacondarwin

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Nov 16, 2019

I have amended the last commit to also escape \ in the "raw" form.
@AndrewKushnir - please can you try the presubmit one more time?

The `:` char is used as a metadata marker in `$localize` messages.
If this char appears in the metadata it must be escaped, as `\:`.
Previously, although the `:` char was being escaped, the TS AST
being generated was not correct and so it was being output double
escaped, which meant that it appeared in the rendered message.

As of TS 3.6.2 the "raw" string can be specified when creating tagged
template AST nodes, so it is possible to correct this.
Since i18n messages are mapped to `$localize` tagged template strings,
the "raw" version must be properly escaped. Otherwise TS will throw an
error such as:

```
Error: Debug Failure. False expression: Expected argument 'text' to be the normalized (i.e. 'cooked') version of argument 'rawText'.
```

This commit ensures that we properly escape these raw strings before creating
TS AST nodes from them.
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:i18n-escape-meta-blocks branch from 4c52d4d to 5e3d9a6 Nov 16, 2019
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Nov 16, 2019

Thanks for the fix @petebacondarwin. I've started new presubmits:

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Nov 18, 2019

Hi @petebacondarwin, VE and Ivy presubmits look good with the most recent changes, so I've set the g3 status to "green" and removed the "blocked" label. Thanks for the fixes 👍

const STRING = /'(\\'|[^'])*'|"(\\"|[^"])*"/;
const BACKTICK_STRING = /\\`(([\s\S]*?)(\$\{[^}]*?\})?)*?\\`/;
const BACKTICK_STRING = /\\`(([\s\S]*?)(\$\{[^}]*?\})?)*?[^\\]\\`/;

This comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 18, 2019

Member

OOC, what does the [^\\] do?

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Nov 18, 2019

Author Member

:-)

This is so that we can pick up escaped single quotes (i.e. \') that exist outside of single quoted strings.

This comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 18, 2019

Member

I see. I guess you don't need the capturing groups (because they will be messed up anyway 😁), right?

Nit: BTW, \$\{[^}]*?\} doesn't need to be non-greedy: \$\{[^}]*\}

alxhub added a commit that referenced this pull request Nov 19, 2019
alxhub added a commit that referenced this pull request Nov 19, 2019
…#33820)

The `:` char is used as a metadata marker in `$localize` messages.
If this char appears in the metadata it must be escaped, as `\:`.
Previously, although the `:` char was being escaped, the TS AST
being generated was not correct and so it was being output double
escaped, which meant that it appeared in the rendered message.

As of TS 3.6.2 the "raw" string can be specified when creating tagged
template AST nodes, so it is possible to correct this.

PR Close #33820
alxhub added a commit that referenced this pull request Nov 19, 2019
…#33820)

Since i18n messages are mapped to `$localize` tagged template strings,
the "raw" version must be properly escaped. Otherwise TS will throw an
error such as:

```
Error: Debug Failure. False expression: Expected argument 'text' to be the normalized (i.e. 'cooked') version of argument 'rawText'.
```

This commit ensures that we properly escape these raw strings before creating
TS AST nodes from them.

PR Close #33820
@alxhub alxhub closed this in 74e6d37 Nov 19, 2019
alxhub added a commit that referenced this pull request Nov 19, 2019
…#33820)

The `:` char is used as a metadata marker in `$localize` messages.
If this char appears in the metadata it must be escaped, as `\:`.
Previously, although the `:` char was being escaped, the TS AST
being generated was not correct and so it was being output double
escaped, which meant that it appeared in the rendered message.

As of TS 3.6.2 the "raw" string can be specified when creating tagged
template AST nodes, so it is possible to correct this.

PR Close #33820
alxhub added a commit that referenced this pull request Nov 19, 2019
…#33820)

Since i18n messages are mapped to `$localize` tagged template strings,
the "raw" version must be properly escaped. Otherwise TS will throw an
error such as:

```
Error: Debug Failure. False expression: Expected argument 'text' to be the normalized (i.e. 'cooked') version of argument 'rawText'.
```

This commit ensures that we properly escape these raw strings before creating
TS AST nodes from them.

PR Close #33820
@petebacondarwin petebacondarwin deleted the petebacondarwin:i18n-escape-meta-blocks branch Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.