diff --git a/src/commands/push.ts b/src/commands/push.ts index 337be5f..e9ff499 100644 --- a/src/commands/push.ts +++ b/src/commands/push.ts @@ -49,9 +49,17 @@ const push = async ( const deleteAbsent = pushOptions?.['delete-absent'] ?? false; const local = await readFiles(localesDir, namespaces); const remote = await apiPull(accessKey, pullOptions); - const { added, deleted, updated, totalCount, deletedCount } = diff(remote, local, pushOptions); + const { added, deleted, updated, totalCount, addedCount, updatedCount, deletedCount } = diff( + remote, + local, + pushOptions + ); - if (!totalCount || (!deleteAbsent && totalCount === deletedCount)) { + if (!totalCount) { + log.success('Everything up to date!'); + return; + } + if (!deleteAbsent && totalCount === deletedCount) { log.info( `Pushing will not delete remote assets when the ${chalk.bold('delete-absent')} flag is disabled` ); @@ -128,13 +136,17 @@ const push = async ( log.log(); - // Loco returns 200 OK with "Nothing updated" when an import is silently - // dropped (most commonly: project has hit its translation quota cap). - // Surface this as a real failure instead of pretending the push succeeded. + // Loco returns 200 OK with "Nothing updated" when an import imports no + // translations. That's expected for deletion-only pushes (delete-absent + // removes assets without "updating" anything), but also fires when an + // import is silently dropped — most commonly when the project has hit + // its translation quota cap. Only treat it as a failure when the diff + // contains actionable add/update entries that Loco should have processed. const allNoOp = responseMessages.every(m => /nothing updated/i.test(m)); - if (allNoOp) { + const hasActionableUpdates = addedCount > 0 || updatedCount > 0; + if (allNoOp && hasActionableUpdates) { log.error( - 'Loco reported no changes despite a non-empty diff. Check your project on https://localise.biz for quota limits or other server-side issues.' + 'Loco reported no changes despite a non-empty diff. Run `loco-cli pull` to reconcile local files with Loco, or check your project on https://localise.biz for quota limits or other server-side issues.' ); throw new CliError('push: no-op'); } diff --git a/src/lib/diff.ts b/src/lib/diff.ts index 4431557..ad7ae5e 100644 --- a/src/lib/diff.ts +++ b/src/lib/diff.ts @@ -27,10 +27,43 @@ export const diff = ( const { added, updated, deleted } = detailedDiff(local, remote) as DetailedDiff; const ignoreAdded = options?.['ignore-new']; const ignoreUpdated = options?.['ignore-existing']; + const experimentalPushAll = options?.experimentalPushAll; + const deleteAbsent = options?.['delete-absent']; const addedRes = ignoreAdded ? {} : dotObject(added); const updatedRes = ignoreUpdated ? {} : dotObject(updated); - const deletedRes = dotObject(deleted); + let deletedRes = dotObject(deleted); + + // With `experimentalPushAll` + `delete-absent`, `apiPushAll` uses + // /import/json?format=multi where `delete-absent` operates on the UNION + // of locales: an asset is only deleted when it's absent from every locale + // in the payload. Recompute the deleted set leaf-by-leaf so the diff + // matches what the upload will actually do — detailedDiff collapses + // missing branches to a single undefined at the branch level, which loses + // the resolution needed to apply union semantics correctly. + // Note: `remote` here is the filesystem-local data, `local` is the + // api-remote data — see the call site in src/commands/push.ts which + // invokes diff(remoteFromApi, localFromFs). + if (experimentalPushAll && deleteAbsent) { + const localFlat: Record = {}; + const unionKeys = new Set(); + for (const [loc, t] of Object.entries(remote)) { + const flat = dotObject(t); + localFlat[loc] = flat; + for (const k of Object.keys(flat)) unionKeys.add(k); + } + deletedRes = {}; + for (const [loc, t] of Object.entries(local)) { + const apiFlat = dotObject(t); + const fsFlat = localFlat[loc] ?? {}; + for (const key of Object.keys(apiFlat)) { + if (!(key in fsFlat) && !unionKeys.has(key)) { + deletedRes[`${loc}.${key}`] = undefined; + } + } + } + } + return { totalCount: Object.keys(addedRes).length + diff --git a/test/commands/push.test.ts b/test/commands/push.test.ts index e4f9158..37f1b52 100644 --- a/test/commands/push.test.ts +++ b/test/commands/push.test.ts @@ -284,6 +284,28 @@ describe('push command', () => { ); }); + test('does not error on "Nothing updated" when the diff is deletion-only', async () => { + // Loco returns "Nothing updated" when no translations are imported, + // including the successful delete-absent case (assets are removed but + // no translations are imported). The allNoOp guard should only fire + // when there are actionable add/update entries. + const local = { en: { hello: 'Hello' } }; + const remote = { en: { hello: 'Hello', bye: 'Goodbye' } }; + mockGetGlobalOptions.mockResolvedValue({ + ...defaultOptions, + push: { 'delete-absent': true } + }); + mockReadFiles.mockResolvedValue(local); + mockApiPull.mockResolvedValue(remote); + mockApiPush.mockResolvedValue({ status: 200, message: 'Nothing updated', locales: [] }); + vi.mocked(mockInquirer.prompt).mockResolvedValue({ confirm: true }); + + await push({}, mockProgram); + + expect(mockLog.success).toHaveBeenCalledWith('All done.'); + expect(mockLog.error).not.toHaveBeenCalled(); + }); + test('does not error when at least one locale reports changes', async () => { const local = { en: { hello: 'Hello' }, es: { hello: 'Hola' } }; const remote = { en: {}, es: {} }; @@ -299,6 +321,39 @@ describe('push command', () => { expect(mockLog.success).toHaveBeenCalledWith('All done.'); }); + test('experimentalPushAll early-exits when only deletions are union-protected', async () => { + // Regression for #56: en has a new key locally, fr/es source-fallback + // remotely. Diff would have flagged fr/es deletions per-locale, but the + // multi-locale upload won't delete them. Expect a clean no-op, not a + // failed push. + const local = { + en: { hello: 'Hello', newKey: 'Value' }, + fr: { hello: 'Bonjour' }, + es: { hello: 'Hola' } + }; + const remote = { + en: { hello: 'Hello', newKey: 'Value' }, + fr: { hello: 'Bonjour', newKey: '' }, + es: { hello: 'Hola', newKey: '' } + }; + mockGetGlobalOptions.mockResolvedValue({ + ...defaultOptions, + push: { + experimentalPushAll: true, + 'ignore-existing': true, + 'delete-absent': true + } + }); + mockReadFiles.mockResolvedValue(local); + mockApiPull.mockResolvedValue(remote); + + await push({}, mockProgram); + + expect(mockLog.success).toHaveBeenCalledWith('Everything up to date!'); + expect(mockApiPushAll).not.toHaveBeenCalled(); + expect(mockApiPush).not.toHaveBeenCalled(); + }); + test('uses default per-locale push when experimentalPushAll is false', async () => { const local = { en: { hello: 'Hello' }, es: { hello: 'Hola' } }; const remote = { en: {}, es: {} }; diff --git a/test/diff.test.ts b/test/diff.test.ts index d199112..dd8fc4d 100644 --- a/test/diff.test.ts +++ b/test/diff.test.ts @@ -13,3 +13,142 @@ test('returns diff without new assets', () => { test('returns diff without updated assets', () => { expect(diff(local, remote, { 'ignore-existing': true })).toMatchSnapshot(); }); + +test('experimentalPushAll filters union-protected deletions', () => { + // Simulates the post-first-push state from issue #56: en has a key, + // fr/es do not locally, but Loco created empty source-fallback entries + // for fr/es. The diff would normally flag fr/es as deletions, but the + // multi-locale upload would not delete them (union-protected by en). + const remoteData = { + en: { hello: 'Hello', newKey: 'Value' }, + fr: { hello: 'Bonjour', newKey: '' }, + es: { hello: 'Hola', newKey: '' } + }; + const localData = { + en: { hello: 'Hello', newKey: 'Value' }, + fr: { hello: 'Bonjour' }, + es: { hello: 'Hola' } + }; + + const result = diff(remoteData, localData, { + experimentalPushAll: true, + 'ignore-existing': true, + 'delete-absent': true + }); + + expect(result.deleted).toEqual({}); + expect(result.deletedCount).toBe(0); + expect(result.totalCount).toBe(0); +}); + +test('experimentalPushAll filters union-protected deletions across missing branches', () => { + // detailedDiff collapses entirely-missing branches (e.g. fs-local nl has + // no E2E subtree at all) into a single undefined at the branch level. + // The fix has to recover leaf-level resolution to apply union semantics. + const remoteData = { + en: { HomeScreen: { Title: 'x' }, E2E: { TestKey: 'val' } }, + nl: { HomeScreen: { Title: 'y' }, E2E: { TestKey: '' } } + }; + const localData = { + en: { HomeScreen: { Title: 'x' }, E2E: { TestKey: 'val' } }, + nl: { HomeScreen: { Title: 'y' } } + }; + + const result = diff(remoteData, localData, { + experimentalPushAll: true, + 'ignore-existing': true, + 'delete-absent': true + }); + + expect(result.deleted).toEqual({}); + expect(result.deletedCount).toBe(0); + expect(result.totalCount).toBe(0); +}); + +test('experimentalPushAll keeps leaf deletions even when sibling leaves are union-protected', () => { + // api-remote has two leaves under a branch (TestKey, OtherKey), but only + // TestKey appears in any local locale. OtherKey is a legitimate union- + // absent deletion that the upload WILL perform, so it must stay in the + // diff. A naive prefix-based filter would over-collapse this case. + const remoteData = { + en: { E2E: { TestKey: 'val', OtherKey: 'gone' } }, + nl: { E2E: { TestKey: '', OtherKey: '' } } + }; + const localData = { + en: { E2E: { TestKey: 'val' } }, + nl: {} + }; + + const result = diff(remoteData, localData, { + experimentalPushAll: true, + 'ignore-existing': true, + 'delete-absent': true + }); + + expect(result.deleted).toEqual({ + 'en.E2E.OtherKey': undefined, + 'nl.E2E.OtherKey': undefined + }); + expect(result.deletedCount).toBe(2); +}); + +test('experimentalPushAll still reports deletions absent from every local locale', () => { + // A key that's been removed from every local locale is a legitimate + // deletion under union semantics — it should remain in the diff. + const remoteData = { + en: { hello: 'Hello', staleKey: 'Old' }, + fr: { hello: 'Bonjour', staleKey: 'Vieux' } + }; + const localData = { + en: { hello: 'Hello' }, + fr: { hello: 'Bonjour' } + }; + + const result = diff(remoteData, localData, { + experimentalPushAll: true, + 'delete-absent': true + }); + + expect(result.deleted).toEqual({ + 'en.staleKey': undefined, + 'fr.staleKey': undefined + }); + expect(result.deletedCount).toBe(2); +}); + +test('experimentalPushAll without delete-absent does not filter (informational only)', () => { + // Without delete-absent, the upload won't delete anything regardless of + // union semantics, so the deleted set is purely informational and should + // be reported per-locale unchanged. + const remoteData = { + en: { hello: 'Hello', newKey: 'Value' }, + fr: { hello: 'Bonjour', newKey: '' } + }; + const localData = { + en: { hello: 'Hello', newKey: 'Value' }, + fr: { hello: 'Bonjour' } + }; + + const result = diff(remoteData, localData, { experimentalPushAll: true }); + + expect(result.deleted).toEqual({ 'fr.newKey': undefined }); + expect(result.deletedCount).toBe(1); +}); + +test('without experimentalPushAll deletions remain per-locale (no union filtering)', () => { + // Per-locale upload path: delete-absent really does apply per locale, + // so the diff must keep per-locale deletions. + const remoteData = { + en: { hello: 'Hello', newKey: 'Value' }, + fr: { hello: 'Bonjour', newKey: '' } + }; + const localData = { + en: { hello: 'Hello', newKey: 'Value' }, + fr: { hello: 'Bonjour' } + }; + + const result = diff(remoteData, localData, { 'delete-absent': true }); + + expect(result.deleted).toEqual({ 'fr.newKey': undefined }); + expect(result.deletedCount).toBe(1); +});