allowEmpty class property not respected in v6.3.1

[IgnaceMaes]

• I am on the latest ember-intl version
• I have searched the issues of this repo and believe that this is not a duplicate

Environment

• Ember Version: 4.12.3
• Ember CLI Version: 4.12.1
• Ember Intl Version: 6.3.1
• Browser(s): Chrome 119
• Node Version: 18

Steps to Reproduce

{{t this.somethingUndefined}}

with a custom T helper as documented here: https://ember-intl.github.io/ember-intl/docs/guide/testing#guarding-against-errors

import Helper from 'ember-intl/helpers/t';

export default class extends Helper {
  allowEmpty = true;
}

Results in Error: {{t}} helper requires a value.. In ember-intl@6.2.2 this worked fine.

As a workaround, I found this works:

import Helper from 'ember-intl/helpers/t';

export default class extends Helper {
  compute(options, namedOptions) {
    return super.compute(options, { allowEmpty: true, ...namedOptions });
  }
}

[ijlee2]

@IgnaceMaes Thanks for reporting a case that I didn't account for in 6.3.0. Namely, an end-developer may override one of the helpers and invert the default value of this.allowEmpty.

In 6.3.0, I removed allowEmpty as a class property, because I found the line options.allowEmpty ?? this.allowEmpty to be confusing (back when ember-intl's helpers inherited a base class). Strangely, {{format-date}} had been the only helper that had set this.allowEmpty to true, so I figured it's unnecessary to have a class property.

---

I think how you overrode the {{t}} helper and passed allowEmpty: true is okay. It may be a brittle solution, since your helper and how you provide types depend on ember-intl's current implementation.

Because there's another regression from 6.3.0 that I want to fix, I can temporarily revert removing the class property this.allowEmpty so that you can still use the syntax,

import THelper from 'ember-intl/helpers/t';

export default class extends THelper {
  allowEmpty = true;
}

Going forward, what we could try is to make all helpers not throw an error when the value provided is either undefined or null, i.e. we replace the lines,

/* In v6.3.1 */
compute([value]) {
  if (isEmpty(value)) {
    if (options?.allowEmpty ?? this.allowEmpty) {
      return '';
    }

    throw new Error('...');
  }

  // ...
}

with,

/* At a later point */
compute([value]) {
  if (value === undefined || value === null) {
    return '';
  }

  // ...
}

Removing isEmpty() would allow me to do type check more easily, and allow end-developers to skip the step of defining options.allowEmpty. I currently don't see a reason for helpers to throw an error (the intl service could still throw an error, e.g. for missing translations).