fix: nested array of objects#14
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to fix flattening behavior so nested arrays containing objects (e.g., questions: [{ title, answer }]) are expanded into indexed keys instead of being coerced to "[object Object]" in generated gettext output.
Changes:
- Update flattening logic to only treat arrays as “array-of-strings” when all elements are strings; otherwise recurse into array elements with an index suffix.
- Regenerate/update the Browserify bundle (
gettext.js) to reflect the flattening change (but it also includes additional unrelated bundle diffs).
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/flatten.js | Adds a string-array guard and recursion for non-string arrays (currently only in the old-format path). |
| gettext.js | Updates the bundled output to include the new array handling, but also includes unrelated dependency/table changes. |
Comments suppressed due to low confidence (1)
gettext.js:8301
- This PR also changes the generated
gettext.jsbundle to include a modifiedsafe-bufferimplementation (new license header / prototype assignment). Since this is not directly related to the flattening logic change, it would be better to avoid unrelated bundle churn (pin deps + rebuild reproducibly) or explicitly justify the dependency update that caused it, as it could have subtle runtime impact in browser builds.
/*! safe-buffer. MIT License. Feross Aboukhadijeh <https://feross.org/opensource> */
/* eslint-disable node/no-deprecated-api */
var buffer = require('buffer')
var Buffer = buffer.Buffer
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else if (Array.isArray(value)) { | ||
| kv = { | ||
| if (value.every(v => typeof v === 'string')) { | ||
| kv = { | ||
| // id: key.replace(new RegExp(' ', 'g'), ''), | ||
| key, | ||
| value: value.join('\n'), | ||
| isArray: true, | ||
| isPlural, | ||
| pluralNumber: isPlural ? number : 0, | ||
| context | ||
| key, | ||
| value: value.join('\n'), |
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Applied the same "array of strings" guard in handleNewFormat so arrays of non-string elements now recurse with indexed keys instead of joining as [object Object]. Also added fixture and test cases for both default and compatibilityJSON: 'v4' modes. Commit: 9f4f02f
| value.forEach((v, i) => { | ||
| recurse(appendTo, v, `${key}${keyseparator}${i}`) | ||
| }) |
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
These tests were already added in commit 9f4f02f: test/fixtures/example-nested-array.json and test/fixtures/example-nested-array.po as fixtures, with assertions for both default and compatibilityJSON: 'v4' modes in test/test.js under the nested array of objects describe block.
…rray tests Agent-Logs-Url: https://github.com/locize/gettext-converter/sessions/07c13542-a6ff-451e-ac53-c05715b7013b Co-authored-by: perrin4869 <5774716+perrin4869@users.noreply.github.com>
|
@adrai when you have time can you publish to npm? Thanks! |
|
thx... |
Ref: i18next/i18next-gettext-converter#428
There were a few false-positive tests in
i18next-gettext-converter, and fixing those tests I found this use-case was broken. Translation files such as:{ "parent": { "questions": [ { "title": "test question", "answer": "test answer" } ] } }would output:
Instead of
The fix is to apply the current logic only if the array is confirmed to be an array of strings