i18next / i18next-parser

Parse your code to extract translation keys/values and manage your catalog files
MIT License
472 stars 192 forks source link

Regression with version 8.0.11 #955

Closed ohardy closed 7 months ago

ohardy commented 7 months ago

πŸ› Bug Report

After upgrade to 8.0.11, this command fail:

i18next 'src/**/*.{ts,tsx}' --config i18next-parser.config.js --fail-on-update --silent

With

> i18next 'src/**/*.{ts,tsx}' --config i18next-parser.config.js --fail-on-update --silent

  [error]   Some keys were sorted and failOnUpdate option is enabled. Exiting...
npm ERR! Lifecycle script `i18n:check` failed with error: 
npm ERR! Error: command failed

Sort is enabled.

If I remove --fail-on-update, no file are updated, so no need to sort anything.

To Reproduce

Run the command above with this config file:


module.exports = {
  contextSeparator: '_',
  createOldCatalogs: false,
  defaultNamespace: 'translation',
  defaultValue: '',
  indentation: 2,
  keepRemoved: false,
  keySeparator: '.',
  lexers: {
    hbs: ['HandlebarsLexer'],
    handlebars: ['HandlebarsLexer'],

    htm: ['HTMLLexer'],
    html: ['HTMLLexer'],

    mjs: ['JavascriptLexer'],
    ts: ['JsxLexer'],
    jsx: ['JsxLexer'],
    tsx: ['JsxLexer'],

    default: ['JavascriptLexer'],
  },
  lineEnding: 'lf',
  locales: ['en', 'fr'],
  namespaceSeparator: ':',
  output: 'src/locales/$LOCALE/$NAMESPACE.json',
  pluralSeparator: '_',
  input: undefined,
  sort: true,
  skipDefaultValues: false,
  useKeysAsDefaultValue: false,
  verbose: false,
  failOnWarnings: false,
  failOnUpdate: false,
  customValueTemplate: null,
  resetDefaultValueLocale: null,
  i18nextOptions: null,
};

Expected behavior

No file need to be sorted so I should not have any error.

Your Environment

kmelancholiy commented 7 months ago

I am also encountering the same error. It seems that in 8.0.11 they added a flag to the key comparison function called from Object.keys(object).sort(compare), but there seems to be a problem. Even if the translation was already sorted, the arguments 1 and 2 were reversed and the result was 1.

γ‚Ήγ‚―γƒͺγƒΌγƒ³γ‚·γƒ§γƒƒγƒˆ 2023-12-30 11 54 35
karellm commented 7 months ago

I'm not entirely sure it is a regression. The project wasn't failing with the fail-on-update flag when the catalog was sorted. Yet, sorting the catalog is an "update" and should trigger a fail. In that sense, I was expecting someone to open a ticket because it is a change in behaviour but it was always supposed to be like that.

In order to debug this, I would need you to provide a failing test case or a project that reproduce the error. I would also invite you to double check that the catalog really hasn't changed.

tec27 commented 7 months ago

This is definitely a regression, not just a change in behavior: this is causing failures when the keys are actually already sorted.

In my project, using this flag on the latest version always fails. If I run the same command without the flag (i.e. to generate the "correct" ordering), it will produce 0 changes to the file. The code that was added to do this seems weird and suspect in quite a number of ways, as instead of sorting the catalog and checking that the ordering matches, it tries to short-circuit the sort operation with an impure compare function (but, the code misunderstands how variable bindings work and thus that also doesn't behave as expected). I'm not sure that we have the guarantees it expects over how this comparison function will be called, and thus it may be machine/version/timing-dependent. Given my tests locally and on CI, however, this gives false negatives 100% of the time.

I can maybe find the time to fix this for you, but this code was clearly suspect even in the PR, and I would appreciate a deeper review process on these things so my project doesn't get broken in the future.

karellm commented 7 months ago

This should be fixed in 8.12.0 thanks to @tec27