extract-i18n does not respect the i18nDuplicateTranslation option

[reduckted]

🐞 Bug report

Command (mark with an x)

• new
• build
• serve
• test
• e2e
• generate
• add
• update
• lint
• extract-i18n
• run
• config
• help
• version
• doc

Is this a regression?

No

Description

Running the extract-i18n command will always log duplicate i18n message IDs as warnings, regardless of the value of the i18nDuplicateTranslation option in the build target.

🔬 Minimal Reproduction

This repository can be used to quickly reproduce this bug:
https://github.com/reduckted/repro-extract-i18n-duplicates

Or using this archive:
repro-extract-i18n-duplicates.zip

1. Run npm i
2. Run ng extract-i18n

The template app.component.html contains two i18n messages with the same ID but different text. The angular.json file has the i18nDuplicateTranslation option set to "error" in the browser builder.

The output is:

WARNINGS:
 - Duplicate messages with id "test":
   - "first" : src\app\app.component.html:1
   - "second" : src\app\app.component.html:2

🔥 Exception or Error

I expect that with i18nDuplicateTranslation set to "error", the duplicate message IDs will be logged as errors and that the process exits with a non-zero exit code.

Instead, the duplicate message IDs are logged as a warning, and the process exits with an exit code of zero.

🌍 Your Environment

> ng version

     _                      _                 ____ _     ___
    / \   _ __   __ _ _   _| | __ _ _ __     / ___| |   |_ _|
   / △ \ | '_ \ / _` | | | | |/ _` | '__|   | |   | |    | |
  / ___ \| | | | (_| | |_| | | (_| | |      | |___| |___ | |
 /_/   \_\_| |_|\__, |\__,_|_|\__,_|_|       \____|_____|___|
                |___/
    

Angular CLI: 14.1.0
Node: 16.12.0
Package Manager: npm 8.15.0
OS: win32 x64

Angular: 14.1.0
... animations, cli, common, compiler, compiler-cli, core, forms
... localize, platform-browser, platform-browser-dynamic, router

Package                         Version
---------------------------------------------------------
@angular-devkit/architect       0.1401.0
@angular-devkit/build-angular   14.1.0
@angular-devkit/core            14.1.0
@angular-devkit/schematics      14.1.0
@schematics/angular             14.1.0
rxjs                            7.5.6
typescript                      4.7.4

Anything else relevant?

Looking at the code, it seems like this might be a simple fix. 🤞

The duplicate message IDs are checked here with a hard-coded value of "warning" and then always logged as warnings:

angular-cli/packages/angular_devkit/build_angular/src/builders/extract-i18n/index.ts

Lines 273 to 283 in 1b89fd4

	const diagnostics = checkDuplicateMessages(
	// eslint-disable-next-line @typescript-eslint/no-explicit-any
	checkFileSystem as any,
	ivyMessages,
	'warning',
	// eslint-disable-next-line @typescript-eslint/no-explicit-any
	basePath as any,
	);
	if (diagnostics.messages.length > 0) {
	context.logger.warn(diagnostics.formatDiagnostics(''));
	}

Earlier in the same function, the browser target options are fetched here, which should contain the value of the i18nDuplicateTranslation option:

angular-cli/packages/angular_devkit/build_angular/src/builders/extract-i18n/index.ts

Lines 138 to 141 in 1b89fd4

	const browserOptions = await context.validateOptions<JsonObject & BrowserBuilderOptions>(
	await context.getTargetOptions(browserTarget),
	await context.getBuilderNameForTarget(browserTarget),
	);

[alan-agius4]

This is expected as the i18nDuplicateTranslation is specific to the browser builder Ie: ng build.

[reduckted]

OK. Will this behavior be changed, or will I need to find a different way to detect duplicate message IDs?

[alan-agius4]

We could expose a similar option to i18nDuplicateTranslation. Let me check with the rest of the team.

[angular-robot]

This feature request is now candidate for our backlog! In the next phase, the community has 60 days to upvote. If the request receives more than 20 upvotes, we'll move it to our consideration list.

You can find more details about the feature request process in our documentation.

[gotwig]

This needs more upvotes Y_Y we need to get rid of those duplicate ID messages

How to solve this? https://stackoverflow.com/questions/70900706/ng-extract-i18n-configuration-to-ignore-duplicate-messages

https://github.com/angular/angular/blob/79620f5139c80f1b3b38168744ca99a224c7e5cd/packages/localize/src/tools/src/extract/main.ts#L89-L95

How to set duplicateMessageHandling?

As a parameter for ng-extract or in some other config? is this in any way configurable?

[dgp1130]

Some discussion about this option today. We definitely agree it should be possible to treat duplicate message IDs as an error during extraction.

What confused us somewhat is why this is an option to begin with? In the event of a duplicate translation which is ignored, I'm not sure we can do anything intelligent to pick between them. Any tool which processes the extracted output won't know which message to use. And when the CLI merges translations back in, we can't know which one is right for the duplicated ID. I double checked with @AndrewKushnir, who also couldn't recall a compelling reason for why a user would want to ignore such duplications.

I'm thinking we should delete the option altogether and just always treat duplicate translation IDs as a hard error, both at extraction and merge times.

[gotwig]

Some discussion about this option today. We definitely agree it should be possible to treat duplicate message IDs as an error during extraction.

What confused us somewhat is why this is an option to begin with? In the event of a duplicate translation which is ignored, I'm not sure we can do anything intelligent to pick between them. Any tool which processes the extracted output won't know which message to use. And when the CLI merges translations back in, we can't know which one is right for the duplicated ID. I double checked with @AndrewKushnir, who also couldn't recall a compelling reason for why a user would want to ignore such duplications.

I'm thinking we should delete the option altogether and just always treat duplicate translation IDs as a hard error, both at extraction and merge times.

But what happens if I want to reuse a key on purpose? The same translation string? :/ I wanted to use the ignore function because its on purpose like I said

[dgp1130]

@gotwig, why do you have/want a duplicate ID? If you want to share a translation in two locations, I think the best approach would be to use $localize and bind that string to two places. Is there another use case I'm not seeing?

[reduckted]

Unless I'm mistaken, I thought you could use the same ID in multiple templates, as long as the text was the same. I know that in the past I've had warnings about duplicate IDs where the only difference between the text was whitespace. Once I fixed the whitespace to be the same, the warning disappeared.

[reduckted]

Just confirming my own thoughts here. With these files:

app.component.html:

<span i18n="@@test-first">First</span>

test.component.html:

<span i18n="@@test-first">First</span>

app.component.html

export class AppComponent {
  title = $localize`:@@test-first:First`;
}

There are no warnings from ng extract-i18n and this is the output:

<?xml version="1.0" encoding="UTF-8" ?>
<xliff version="1.2" xmlns="urn:oasis:names:tc:xliff:document:1.2">
  <file source-language="en-US" datatype="plaintext" original="ng2.template">
    <body>
      <trans-unit id="test-first" datatype="html">
        <source>First</source>
        <context-group purpose="location">
          <context context-type="sourcefile">src/app/app.component.html</context>
          <context context-type="linenumber">1</context>
        </context-group>
        <context-group purpose="location">
          <context context-type="sourcefile">src/app/app.component.ts</context>
          <context context-type="linenumber">9</context>
        </context-group>
        <context-group purpose="location">
          <context context-type="sourcefile">src/app/test/test.component.html</context>
          <context context-type="linenumber">1</context>
        </context-group>
      </trans-unit>
    </body>
  </file>
</xliff>

However, if I add some whitespace to one of the values, for example:

<span i18n="@@test-first"> First </span>

Then you get a warning, which is 100% what I would expect (although it should be an error 😆)

WARNINGS:
 - Duplicate messages with id "test-first":
   - "First" : src\app\app.component.ts:9
   - " First " : src\app\app.component.html:1
   - "First" : src\app\test\test.component.html:1

So you can use the same ID in multiple templates. You just have to make sure they have the same text value. If they don't have the same text value, then you absolutely should be alerted about it via an error.

[gotwig]

@reduckted : nice example, great :) And what about the idea of specifying the base string once and then only using the @@id in the places you reuse the value? I guess this goes more towards the direction of ngx-translate:

app.component.html:
<span i18n="@@test-first">First</span>

test.component.html
<span i18n="@@test-first"></span>

[dgp1130]

@reduckted, I don't think our behavior needs to change there. Just like how no warning is emitted when the text matches, we don't have to fail that case. My suggestion is to always error in the case where there are two messages with the same ID and different content.

On a separate point, if you have two locations where you want to use the same translations, I recommend using $localize instead at one place in your code and just binding it in two locations. That way you have a single source of truth and you don't have to hard-code an ID anywhere.

[reduckted]

Yes, I absolutely agree. 👍

[angular-robot]

Just a heads up that we kicked off a community voting process for your feature request. There are 20 days until the voting process ends.

Find more details about Angular's feature request process in our documentation.

[angular-robot]

Thank you for submitting your feature request! Looks like during the polling process it didn't collect a sufficient number of votes to move to the next stage.

We want to keep Angular rich and ergonomic and at the same time be mindful about its scope and learning journey. If you think your request could live outside Angular's scope, we'd encourage you to collaborate with the community on publishing it as an open source package.

You can find more details about the feature request process in our documentation.

[vbraun]

Workaround is to put

npx localize-extract --format=xlf --source=./dist/app/**/*.js --outputPath=/dev/null --duplicateMessageHandling=error

into your CI, will exit with status 1 on duplicates

[superole]

I am hoping that since this issue is not closed, it will eventually be fixed. It would be nice to be able to break builds that introduce i18n bugs.

[telb99]

This is still a problem for us in Angular 17.3

We are looking at implementing a redirect of the output from running "ng extract-i18n" to a file and then searching for 'Duplicate messages' and aborting our CI build if found.

Not ideal, and brittle if that message text ever changes.

[reduckted]

We are looking at implementing a redirect of the output from running "ng extract-i18n" to a file and then searching for 'Duplicate messages' and aborting our CI build if found.

That's pretty much what I had to end up doing. I created a custom builder that intercepts console.log calls then calls the original builder. If a console message about duplicate i18n messages is received, it sets the process exit code to 1.

I'm amazed this hasn't been fixed after all this time. Surely if you're extracting i18n messages to send away to be translated, you'd want to know if there were duplicates. 🤷‍♂️

[spock123]

We are heavily using duplicates and it's pretty annoying in development mode, to have hundreds of "duplicate message id" warnings all the time. We should not rely on having to write precisely the same filler text, at least give us an option to ignore duplicate warnings.