-
Notifications
You must be signed in to change notification settings - Fork 24
fix(cli): Fix composite JSON #1108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| 'gt': patch | ||
| --- | ||
|
|
||
| Fix string behavior |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,7 +62,7 @@ export function parseJson( | |
| // Construct lvl 2 | ||
| const sourceObjectsToTranslate: Record< | ||
| string, | ||
| Record<string, Record<string, string>> | ||
| Record<string, Record<string, string>> | string | ||
| > = {}; | ||
|
Comment on lines
63
to
66
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No tests for new string handling There are no test cases in Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/cli/src/formats/json/parseJson.ts
Line: 63-66
Comment:
**No tests for new string handling**
There are no test cases in `parseJson.test.ts`, `mergeJson.test.ts`, or `extractJson.test.ts` covering the new string-value composite path (e.g. `{ translations: { en: "Hello", fr: "Bonjour" } }`). Since this PR is specifically fixing a bug in this code path, a regression test covering the full parse → merge → extract round-trip for string-valued composite schemas would significantly improve confidence in the fix.
How can I resolve this? If you propose a fix, please make it concise. |
||
| for (const [ | ||
| sourceObjectPointer, | ||
|
|
@@ -163,6 +163,12 @@ export function parseJson( | |
| } | ||
| const { sourceItem } = matchingItem; | ||
|
|
||
| // If the source item is a string, use it directly | ||
| if (typeof sourceItem === 'string') { | ||
| sourceObjectsToTranslate[sourceObjectPointer] = sourceItem; | ||
| continue; | ||
| } | ||
|
|
||
| // Get the fields to translate from the includes | ||
| const flatten = filterStrings ? flattenJsonWithStringFilter : flattenJson; | ||
| const itemsToTranslate = flatten(sourceItem, sourceObjectOptions.include); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,2 @@ | ||
| // This file is auto-generated. Do not edit manually. | ||
| export const PACKAGE_VERSION = '2.10.0'; | ||
| export const PACKAGE_VERSION = '2.10.1'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
targetItemsreplaced before string checkThe
!targetItemsguard on line 331–333 replaces a falsytargetItemswith{}before the new string path is reached. An empty-string translation ("") is falsy, so it gets silently overwritten with{}, causingtypeof targetItems === 'string'to befalseand the code to fall back todefaultLocaleSourceItem. This means an empty translated string in the source map (intentional or otherwise) is silently discarded and replaced with the default-locale value.Consider tightening the guard to only replace
null/undefined, not empty strings:The real fix belongs at the
!targetItemsinitialisation (line 331):This keeps
""as a string so the downstreamtypeof targetItems === 'string'check works correctly.Prompt To Fix With AI