i18next / i18next-http-backend

i18next-http-backend is a backend layer for i18next using in Node.js, in the browser and for Deno.
MIT License
453 stars 70 forks source link

Documentation regarding API interface for backend `request` option is inaccurate #21

Closed jerrypopsoff closed 4 years ago

jerrypopsoff commented 4 years ago

🐛 Bug Report

I had to step through the invocation of callback from my application code to figure out that data property indicated within the res object should be stringified JSON rather than an Object instance. It also is worth noting that there was no indication of error in this case until attempting to look up the translation for a key (e.g. "i18next::translator: missingKey ...").

Perhaps it would help to provide an example of overriding request in the examples/ within the repository.

The documentation in question (from the README):

  // define a custom request function
  // can be used to support XDomainRequest in IE 8 and 9
  //
  // 'options' will be this entire options object
  // 'url' will be passed the value of 'loadPath'
  // 'payload' will be a key:value object used when saving missing translations
  // 'callback' is a function that takes two parameters, 'err' and 'res'.
  //            'err' should be an error
  //            'res' should be an object with a 'status' property and a 'data' property the key:value translation pairs for the
  //            requested language and namespace, or null in case of an error.
  request: function (options, url, payload, callback) {},

To Reproduce

Override request option with something like the following:

request: async (
  _options: object,
  _url: string,
  _payload: object,
  callback: (err?: object, res?: object) => never,
 ): Promise<never> => {
    const en = await import('./locales/en.json');
    callback(undefined, { data: en.default });
    // callback(undefined, { data: JSON.stringify(en.default) }); // "Correct" code
  };
}

Expected behavior

Bare minimum: update the documentation on the README:

  // define a custom request function
  // can be used to support XDomainRequest in IE 8 and 9
  //
  // 'options' will be this entire options object
  // 'url' will be passed the value of 'loadPath'
  // 'payload' will be a key:value object used when saving missing translations
  // 'callback' is a function that takes two parameters, 'err' and 'res'.
  //            'err' should be an error
  //            'res' should be an object with a 'status' property and a 'data' property
  //            containing a stringified object instance containing the key:value translation pairs for the
  //            requested language and namespace, or null in case of an error.
  request: function (options, url, payload, callback) {},

Stretch goal: Also ensure an error is thrown and/or logged when parsing fails via the default behavior (with only request overriden). I don't believe this callback invocation results in consuming application code from being notified of an issue (unless I have something misconfigured). https://github.com/i18next/i18next-http-backend/blob/ead886f5c23fe20064bfc299d4d6ebeebfeeaeb2/lib/index.js#L75

adrai commented 4 years ago

1) documentation is updated 2) with v1.0.17 there is now a fallback, if data is not a string, so your use case should work without stringifying 3) the error passed via callback is logged via i18next module