Skip to content

Conversation

@codestacx
Copy link

    # PR Review: Support per-call defaultValue for missing translations

Summary

  • Adds a per-call default value capability to i18n.__ so callers can provide { defaultValue: '...' } when a translation key is missing.
  • Conceptually sound and aligns with the issue request. Documentation and tests are included.
  • However, the patch introduces unrelated behavioral changes (deferred file writes) and places tests under a non-standard directory, which can break CI/test discovery.

Key Findings

  • Tests directory mismatch: tests/i18n.test.js may not be discovered by tap. Conventionally, tap looks under test/.
  • Introduces asynchronous/deferred writes (writeQueue + process.nextTick), changing existing semantics where updateFiles writes immediately. This is a potential backward-incompatible change and adds complexity with little benefit.
  • Unnecessary refactors in i18n.js (writeQueue helpers, api.__ wrapper) increase risk of merge conflicts without impacting the feature.
  • The feature itself should be a minimal change: parse defaultValue from the last argument when it’s an object, remove it from interpolation options, and use it if the key is missing.
  • README updates are aligned with the feature, but should emphasize no interpolation of defaultValue and persistence behavior only when updateFiles is true.

Recommendations (Concrete Fixes)

  • Move tests to test/ (or update package.json test script to include tests/), e.g., test/default-value.test.js.
  • Revert to synchronous write behavior (call writeFile(locale) immediately) to preserve existing semantics and compatibility.
  • Keep api.__ implementation unchanged (use translate.apply(null, arguments)).
  • Use hasOwnProperty to detect whether a key is present, so empty-string translations are not treated as missing.
  • Keep the diff minimal, focusing only on defaultValue extraction and fallback behavior.

Code Changes

Changes to: i18n.js

--- a/i18n.js
+++ b/i18n.js
@@
-  function translate(phrase, args) {
+  function translate(phrase, args) {
@@
-    args = Array.prototype.slice.call(arguments, 1);
+    args = Array.prototype.slice.call(arguments, 1);
+
+    // Support per-call default value: __('key', { defaultValue: '...' })
+    // or __('key', 123, { defaultValue: '...' }) when using sprintf-style args
+    var defaultValue;
+    if (args.length) {
+      var last = args[args.length - 1];
+      if (last && typeof last === 'object' && Object.prototype.hasOwnProperty.call(last, 'defaultValue')) {
+        defaultValue = last.defaultValue;
+        // remove defaultValue from interpolation object
+        var cleaned = {};
+        Object.keys(last).forEach(function (k) {
+          if (k !== 'defaultValue') cleaned[k] = last[k];
+        });
+        if (Object.keys(cleaned).length) {
+          args[args.length - 1] = cleaned;
+        } else {
+          args.pop();
+        }
+      }
+    }
@@
-    var translation = locales[locale] && locales[locale][phrase];
+    var hasTranslation = locales[locale] && Object.prototype.hasOwnProperty.call(locales[locale], phrase);
+    var translation = hasTranslation ? locales[locale][phrase] : undefined;
@@
-    if (!translation) {
-      if (options.updateFiles && (typeof phrase === 'string' || typeof phrase === 'number')) {
-        if (!locales[locale]) locales[locale] = {};
-        locales[locale][phrase] = options.missingKeyFn(locale, phrase);
-        writeFile(locale);
-      }
-      translation = options.missingKeyFn(locale, phrase);
-    }
+    if (!hasTranslation) {
+      var missing = defaultValue !== undefined ? defaultValue : options.missingKeyFn(locale, phrase);
+      if (options.updateFiles && (typeof phrase === 'string' || typeof phrase === 'number')) {
+        if (!locales[locale]) locales[locale] = {};
+        locales[locale][phrase] = missing;
+        writeFile(locale);
+      }
+      translation = missing;
+    }

Changes to: README.md

--- a/README.md
+++ b/README.md
@@
+### Default value for missing translations
+
+When a translation key is not found in the active locale, i18n normally returns the key itself and (if `updateFiles` is enabled) writes the missing key to the locale file with the key as value.
+
+You can override this behavior per call by providing a `defaultValue` that will be returned and, if `updateFiles` is enabled, written to the locale file instead of the key.
+
+Examples:
+
+```js
+// returns 'default_value' if 'hello' is missing and writes it to the current locale file when updateFiles is true
+i18n.__('hello', { defaultValue: 'default_value' });
+
+// with sprintf-style interpolation
+i18n.__('welcome %s', 'John', { defaultValue: 'Welcome!' });
+
+// object form still works too
+i18n.__({ phrase: 'hello', locale: 'en' }, { defaultValue: 'default_value' });
+```
+
+Behavior details:
+- If the key exists, the stored translation is returned as usual.
+- If the key is missing and `defaultValue` is provided, `defaultValue` is returned.
+- If `updateFiles` is enabled and the key is missing, the stored value will be:
+  - `defaultValue` when provided.
+  - The result of `missingKeyFn(locale, key)` otherwise (defaults to the key).
+
+Note: if you pass an options object for interpolation values, `defaultValue` is treated as a reserved option and is not used as an interpolation variable. It is removed from the interpolation object before formatting.
+
+```js
+// defaultValue is not interpolated into the string
+i18n.__('hi {name}', { name: 'Ada', defaultValue: 'Hello' }); // -> uses 'Hello' only if key is missing
+```

New file: test/default-value.test.js

--- /dev/null
+++ b/test/default-value.test.js
@@ -0,0 +1,98 @@
+'use strict';
+
+const fs = require('fs');
+const path = require('path');
+const os = require('os');
+const t = require('tap');
+const i18n = require('..');
+
+function tmpDir() {
+  return fs.mkdtempSync(path.join(os.tmpdir(), 'i18n-node-'));
+}
+
+function readJSON(file) {
+  return JSON.parse(fs.readFileSync(file, 'utf8'));
+}
+
+t.test('defaultValue is returned when key is missing', (t) => {
+  const dir = tmpDir();
+  i18n.configure({
+    locales: ['en'],
+    directory: dir,
+    updateFiles: false,
+    objectNotation: false
+  });
+
+  const res = i18n.__('missing_key', { defaultValue: 'default_value' });
+  t.equal(res, 'default_value', 'returns provided defaultValue');
+  t.end();
+});
+
+t.test('defaultValue is persisted when updateFiles is true', (t) => {
+  const dir = tmpDir();
+  i18n.configure({
+    locales: ['en'],
+    directory: dir,
+    updateFiles: true,
+    objectNotation: false
+  });
+
+  const file = path.join(dir, 'en.json');
+  const key = 'hello';
+  const def = 'default_value';
+  const res = i18n.__(key, { defaultValue: def });
+  t.equal(res, def, 'returns defaultValue');
+
+  t.equal(fs.existsSync(file), true, 'en.json is written');
+  const data = readJSON(file);
+  t.same(data[key], def, 'defaultValue persisted to file for missing key');
+  t.end();
+});
+
+t.test('existing keys ignore defaultValue and return stored translation', (t) => {
+  const dir = tmpDir();
+  i18n.configure({ locales: ['en'], directory: dir, updateFiles: true });
+
+  const file = path.join(dir, 'en.json');
+  fs.writeFileSync(file, JSON.stringify({ greeting: 'Hola' }, null, 2));
+  // reload to pick up the file
+  i18n.configure({ locales: ['en'], directory: dir, updateFiles: true });
+
+  const res = i18n.__('greeting', { defaultValue: 'Hello' });
+  t.equal(res, 'Hola', 'uses stored translation, not defaultValue');
+  t.end();
+});
+
+t.test('without defaultValue, missingKeyFn/key continues to be used', (t) => {
+  const dir = tmpDir();
+  i18n.configure({ locales: ['en'], directory: dir, updateFiles: true });
+
+  const res = i18n.__('missing_key_2');
+  t.equal(res, 'missing_key_2', 'falls back to key by default');
+  t.end();
+});
+
+t.test('sprintf args and defaultValue work together', (t) => {
+  const dir = tmpDir();
+  i18n.configure({ locales: ['en'], directory: dir, updateFiles: true });
+
+  const res = i18n.__('welcome %s', 'Ada', { defaultValue: 'Welcome!' });
+  t.equal(res, 'Welcome!', 'uses defaultValue when key is missing despite sprintf args being provided');
+  t.end();
+});

Usage Examples

  • Per-call default for a missing key:
    • i18n.__('hello', { defaultValue: 'default_value' }) // returns 'default_value' if 'hello' is missing; persists it when updateFiles is true
  • With sprintf-style interpolation arguments:
    • i18n.__('greet %s', 'Jane', { defaultValue: 'Hi there!' }) // returns 'Hi there!' if 'greet %s' is missing
  • Object form with locale override:
    • i18n.__({ phrase: 'tagline', locale: 'en' }, { defaultValue: 'The default tagline' })

How to test:

  1. Install deps and run tests: npm i && npm test
  2. Manual check:
    • Configure i18n with updateFiles: true and directory pointing to a temporary folder.
    • Call i18n.__('missing_key', { defaultValue: 'default_value' }) and verify en.json contains the key/value and the function returns 'default_value'.
    • Create a locale file with an existing key and verify defaultValue is ignored when the key exists.

Conclusion

  • The feature is valuable and fits the project. With the above adjustments (tests path, keep synchronous writes, minimal diff), the change will be safer to merge and maintain backward compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant