I ran a presubmit and got some duplicate message errors. Debugging further, I see a problem with how we're generating this code.

Context

Internally, i18n messages outside of Angular templates are defined via Closure's goog.getMsg API (roughly equivalent to Angular's $localize).

/** @desc Say hello. */
const MSG_HELLO = goog.getMsg('Hello, Alex!');
console.log(MSG_HELLO); // 'Hello, Alex!' (or localized equivalent)

It's somewhat reasonable to parameterize such messages in a computed like so:

const name = signal('Alex');
const hello = computed(() => {
  /** @desc Say hello. */
  const MSG_HELLO = goog.getMsg('Hello, {$name}', {name: name()});
  return MSG_HELLO;
});

Bug

Running the transform on this, the body of the computed gets duplicated like so:

const name = signal(...(ngDevMode ? ['Alex', {debugName: 'name'}] : ['Alex']));
const hello = computed(...(ngDevMode ? [
  () => {
    /** @desc Say hello. */
    const MSG_HELLO = goog.getMsg('Hello, {$name}', {name: name()});
    return MSG_HELLO;
  },
  {debugName: 'hello'},
] : [
  () => {
    /** @desc Say hello. */
    const MSG_HELLO = goog.getMsg('Hello, {$name}', {name: name()});
    return MSG_HELLO;
  },
]);

Note that we now have two goog.getMsg calls. Closure Compiler extracts all i18n messages in the bundle by analyzing the AST, meaning it now extracts two MSG_HELLO symbols and errors because of the conflict. It would not be able to disambiguate between the two. I wonder if $localize might actually have a similar problem. 🤔

We could ask Closure Compiler to be a little more lenient here, as the two messages are literally identical, it could probably make this work. However I do expect that erroring on duplicate messages is a generally reasonable thing for the compiler to do and I wouldn't want to turn that off just because of this case. Looking at this code it is also clear that we're duplicating a significant amount of code in the computed expression and dev builds typically run uncompiled, meaning both branches are shipped to the browser (even if only one is executed), so this is a potentially significant performance regression for developers, even if it ultimately gets compiled out for production users.

I remember we had some discussion about how safe it was to duplicate the expression in a signal and whether that might produce duplicated side effects. I think we ultimately decided it didn't because the ternary short circuits and only the true branch gets evaluated at runtime. However I think this issue makes it clear that duplicating the code at all, even if never executed, is still a potential issue.

Solution 1

I think the path forward here is to avoid duplicating the expression by putting it in a variable used in both branches of the ngDevMode ternary. My initial attempt was to use an IIFE using an inlined parameter like so:

const name = signal(...(ngDevMode ? ['Alex', {debugName: 'name'}] : ['Alex']));
const hello = computed(...(
  (msg) => ngDevMode
    ? [msg, {debugName: 'hello'}]
    : [msg]
)(() => {
  /** @desc Say hello. */
  const MSG_HELLO = goog.getMsg('Hello, {$name}', {name: name()});
  return MSG_HELLO;
});

This keeps a single definition of the function and only one goog.getMsg while ultimately evaluating to the same thing based on ngDevMode.

The problem with this is that it might be just a little too complicated an expression for bundlers. Testing this out I see:

// Ideally:
computed(() => "Hello, Alex!");

// Closure Compiler:
computed(() => "Hello, Alex!");

// Angular CLI: esbuild + terser
computed(...(t=>[t])(()=>"Hello, Alex!"));

So Closure does seem smart enough to make this work, but esbuild and Terser are coming up a little short. I suspect the problem is that esbuild/Terser don't want to inline t in the IIFE because if t is used multiple times the expression would be duplicated. It's only safe to inline that variable because it's only used once in t=>[t]. Based on that, we can maybe rework this to explicitly put the computed function into a variable so the bundler doesn't need to inline an IIFE with a parameter.

Solution 2

const hello = (() => {
  const x = () => {
    /** @desc Say hello. */
    const MSG_HELLO = goog.getMsg('Hello, Alex!');
    return MSG_HELLO;
  };
  return computed(...(ngDevMode ? [x, {debugName: 'hello'}] : [x]);
})();

This outputs:

// Closure Compiler:
(() => computed(() => "Hello world"))();

// Angular CLI: esbuild + Terser
computed(()=>"Hello, World!");

So now we've got an extra IIFE in Closure which is failing to be inlined. It's a little better than this doesn't have a spread operator in the computed call, which is likely a runtime perf deoptimization as the engine probably needs to do extra work to expand the array.

I'm not sure why Closure Compiler can't inline that IIFE, especially since it inlined a much more complicated IIFE in my previous attempt. I'll ask around and see if we can find a pattern which optimizes ideally for both sets of bundlers.

Hmm, retrying the second solution I'm actually now seeing that it outputs computed(() => 'Hello, Alex!') in both cases, I'm not sure why my initial testing came up different.

If that's reliable then it seems like that transform would be valid path forward, I'll keep an eye out if I can replicate the extra IIFE I saw previously.

Global presubmit is fully passing 🎉, so I think this output format is looking good.

http://test/OCL:728286513:BASE:728422677:1739934312484:4bfbb98b

Passing TGP: http://test/OCL:749909832:BASE:750657379:1745431857014:f318b2d7

TGP still looking good: http://test/OCL:758440362:BASE:758744767:1747244383957:e4a093db

This PR was merged into the repository by commit 3a9a70d.

The changes were merged into the following branches: main

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}