Skip to content

Commit bdac322

Browse files
rexxarsclaude
andcommitted
refactor(cli-core): remove configstore dependency, use sync config I/O
Replace the configstore-backed getUserConfig() with a dependency-free implementation that reads/writes the CLI config JSON file directly using sync I/O. This fixes a race condition in logout where concurrent async read-modify-write cycles on the same config file could silently lose data (e.g. telemetry consent surviving a logout). The getUserConfig() public API keeps its name. The return type narrows from configstore's ConfigStore class to a minimal ConfigStore interface with get/set/delete. Moves telemetry utilities from src/util/ to src/telemetry/ in cli-core. Removes 6 transitive packages. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 7af617a commit bdac322

File tree

27 files changed

+327
-254
lines changed

27 files changed

+327
-254
lines changed

packages/@sanity/cli-core/package.json

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,6 @@
6868
"@sanity/client": "catalog:",
6969
"babel-plugin-react-compiler": "^1.0.0",
7070
"boxen": "^8.0.1",
71-
"configstore": "^7.0.0",
7271
"debug": "catalog:",
7372
"get-it": "^8.7.0",
7473
"get-tsconfig": "catalog:",

packages/@sanity/cli-core/src/SanityCommand.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import {
1818
} from './services/apiClient.js'
1919
import {type CLITelemetryStore} from './telemetry/types.js'
2020
import {type Output} from './types.js'
21-
import {getCliTelemetry, reportCliTraceError} from './util/getCliTelemetry.js'
21+
import {getCliTelemetry, reportCliTraceError} from './telemetry/getCliTelemetry.js'
2222
import {isInteractive} from './util/isInteractive.js'
2323

2424
type Flags<T extends typeof Command> = Interfaces.InferredFlags<
Lines changed: 160 additions & 88 deletions
Original file line numberDiff line numberDiff line change
@@ -1,110 +1,92 @@
1-
import {mkdir} from 'node:fs/promises'
1+
import {mkdirSync} from 'node:fs'
22
import {homedir} from 'node:os'
33

44
import {afterEach, beforeEach, describe, expect, test, vi} from 'vitest'
55

6-
import {getCliUserConfig, setCliUserConfig} from '../../services/cliUserConfig'
7-
import {readJsonFile} from '../../util/readJsonFile'
8-
import {writeJsonFile} from '../../util/writeJsonFile'
6+
import {getCliUserConfig, getUserConfig, setCliUserConfig} from '../../services/cliUserConfig'
7+
import {readJsonFileSync} from '../../util/readJsonFileSync'
8+
import {writeJsonFileSync} from '../../util/writeJsonFileSync'
99

10-
vi.mock('node:fs/promises')
10+
vi.mock('node:fs')
1111
vi.mock('node:os')
12-
vi.mock('../../util/readJsonFile')
13-
vi.mock('../../util/writeJsonFile')
12+
vi.mock('../../util/readJsonFileSync')
13+
vi.mock('../../util/writeJsonFileSync')
1414

1515
const mockHomedir = '/mock/home/dir'
1616

1717
describe('cliUserConfig', () => {
1818
beforeEach(() => {
1919
vi.resetAllMocks()
2020
vi.mocked(homedir).mockReturnValue(mockHomedir)
21-
vi.mocked(mkdir).mockResolvedValue(undefined)
22-
vi.mocked(readJsonFile).mockResolvedValue({})
23-
vi.mocked(writeJsonFile).mockResolvedValue()
21+
vi.mocked(mkdirSync).mockReturnValue(undefined)
22+
vi.mocked(readJsonFileSync).mockReturnValue({})
23+
vi.mocked(writeJsonFileSync).mockReturnValue()
2424
})
2525

2626
afterEach(() => {
27-
vi.resetAllMocks()
27+
vi.clearAllMocks()
2828
})
2929

3030
describe('readConfig behavior', () => {
31-
test('returns empty config when file read fails', async () => {
32-
vi.mocked(readJsonFile).mockRejectedValueOnce(new Error('File not found'))
33-
const result = await getCliUserConfig('authToken')
31+
test('returns empty config when file read fails', () => {
32+
vi.mocked(readJsonFileSync).mockImplementationOnce(() => {
33+
throw new Error('File not found')
34+
})
35+
const result = getCliUserConfig('authToken')
3436
expect(result).toBeUndefined()
3537
})
3638

37-
test('returns empty config when file content is null', async () => {
38-
vi.mocked(readJsonFile).mockResolvedValueOnce(null)
39-
const result = await getCliUserConfig('authToken')
39+
test('returns empty config when file content is null', () => {
40+
vi.mocked(readJsonFileSync).mockReturnValueOnce(null)
41+
const result = getCliUserConfig('authToken')
4042
expect(result).toBeUndefined()
4143
})
4244

43-
test('returns empty config when file content is an array', async () => {
44-
vi.mocked(readJsonFile).mockResolvedValueOnce([])
45-
const result = await getCliUserConfig('authToken')
45+
test('returns empty config when file content is an array', () => {
46+
vi.mocked(readJsonFileSync).mockReturnValueOnce([])
47+
const result = getCliUserConfig('authToken')
4648
expect(result).toBeUndefined()
4749
})
4850

49-
test('returns empty config when file content is not an object', async () => {
50-
vi.mocked(readJsonFile).mockResolvedValueOnce('not an object')
51-
const result = await getCliUserConfig('authToken')
51+
test('returns empty config when file content is not an object', () => {
52+
vi.mocked(readJsonFileSync).mockReturnValueOnce('not an object')
53+
const result = getCliUserConfig('authToken')
5254
expect(result).toBeUndefined()
5355
})
5456
})
5557

5658
describe('getCliUserConfig', () => {
57-
test('returns authToken when valid', async () => {
58-
vi.mocked(readJsonFile).mockResolvedValueOnce({
59+
test('returns authToken when valid', () => {
60+
vi.mocked(readJsonFileSync).mockReturnValueOnce({
5961
authToken: 'test-token',
6062
})
6163

62-
const result = await getCliUserConfig('authToken')
64+
const result = getCliUserConfig('authToken')
6365
expect(result).toBe('test-token')
6466
})
6567

66-
test('returns undefined when authToken is not set', async () => {
67-
vi.mocked(readJsonFile).mockResolvedValueOnce({})
68+
test('returns undefined when authToken is not set', () => {
69+
vi.mocked(readJsonFileSync).mockReturnValueOnce({})
6870

69-
const result = await getCliUserConfig('authToken')
71+
const result = getCliUserConfig('authToken')
7072
expect(result).toBeUndefined()
7173
})
7274

73-
test('returns telemetryConsent when valid', async () => {
74-
const mockConsent = {
75-
updatedAt: Date.now(),
76-
value: {
77-
status: 'granted',
78-
type: 'explicit',
79-
},
80-
}
81-
vi.mocked(readJsonFile).mockResolvedValueOnce({
82-
telemetryConsent: mockConsent,
83-
})
84-
85-
const result = await getCliUserConfig('telemetryConsent')
86-
expect(result).toEqual(mockConsent)
87-
})
88-
89-
test('throws error for invalid property', async () => {
90-
await expect(getCliUserConfig('invalidProp' as never)).rejects.toThrow('No schema defined')
91-
})
92-
93-
test('returns undefined for invalid value type', async () => {
94-
vi.mocked(readJsonFile).mockResolvedValueOnce({
75+
test('returns undefined for invalid value type', () => {
76+
vi.mocked(readJsonFileSync).mockReturnValueOnce({
9577
authToken: 123, // Invalid type, should be string
9678
})
9779

98-
await expect(getCliUserConfig('authToken')).resolves.toBeUndefined()
80+
expect(getCliUserConfig('authToken')).toBeUndefined()
9981
})
10082
})
10183

10284
describe('setCliUserConfig', () => {
103-
test('sets valid authToken', async () => {
104-
await setCliUserConfig('authToken', 'new-token')
85+
test('sets valid authToken', () => {
86+
setCliUserConfig('authToken', 'new-token')
10587

106-
expect(mkdir).toHaveBeenCalledWith(expect.any(String), {recursive: true})
107-
expect(writeJsonFile).toHaveBeenCalledWith(
88+
expect(mkdirSync).toHaveBeenCalledWith(expect.any(String), {recursive: true})
89+
expect(writeJsonFileSync).toHaveBeenCalledWith(
10890
expect.any(String),
10991
expect.objectContaining({
11092
authToken: 'new-token',
@@ -113,58 +95,148 @@ describe('cliUserConfig', () => {
11395
)
11496
})
11597

116-
test('sets valid telemetryConsent', async () => {
117-
const mockConsent = {
118-
updatedAt: Date.now(),
119-
value: {
120-
status: 'granted' as const,
121-
type: 'explicit',
122-
},
123-
}
98+
test('throws error for invalid value type', () => {
99+
expect(() => setCliUserConfig('authToken', 123 as never)).toThrow('Invalid value')
100+
})
101+
102+
test('merges new config with existing config', () => {
103+
vi.mocked(readJsonFileSync).mockReturnValueOnce({
104+
authToken: 'existing-token',
105+
someOtherKey: 'preserved',
106+
})
124107

125-
await setCliUserConfig('telemetryConsent', mockConsent)
108+
setCliUserConfig('authToken', 'new-token')
126109

127-
expect(writeJsonFile).toHaveBeenCalledWith(
110+
expect(writeJsonFileSync).toHaveBeenCalledWith(
128111
expect.any(String),
129112
expect.objectContaining({
130-
telemetryConsent: mockConsent,
113+
authToken: 'new-token',
114+
someOtherKey: 'preserved',
131115
}),
132116
expect.any(Object),
133117
)
134118
})
119+
})
120+
121+
describe('getUserConfig', () => {
122+
test('get returns raw value from file', () => {
123+
vi.mocked(readJsonFileSync).mockReturnValueOnce({
124+
myKey: 'myValue',
125+
nested: {deep: true},
126+
})
127+
128+
const store = getUserConfig()
129+
const result = store.get('myKey')
130+
expect(result).toBe('myValue')
131+
})
132+
133+
test('get returns undefined for missing key', () => {
134+
vi.mocked(readJsonFileSync).mockReturnValueOnce({})
135+
136+
const store = getUserConfig()
137+
const result = store.get('nonexistent')
138+
expect(result).toBeUndefined()
139+
})
140+
141+
test('get returns undefined when file read fails', () => {
142+
vi.mocked(readJsonFileSync).mockImplementationOnce(() => {
143+
throw new Error('File not found')
144+
})
145+
146+
const store = getUserConfig()
147+
const result = store.get('anyKey')
148+
expect(result).toBeUndefined()
149+
})
150+
151+
test('set reads file, adds key, and writes file', () => {
152+
vi.mocked(readJsonFileSync).mockReturnValueOnce({
153+
existing: 'value',
154+
})
135155

136-
test('throws error for invalid property', async () => {
137-
// eslint-disable-next-line @typescript-eslint/no-explicit-any
138-
await expect(setCliUserConfig('invalidProp' as any, 'value')).rejects.toThrow(
139-
'No schema defined',
156+
const store = getUserConfig()
157+
store.set('newKey', 'newValue')
158+
159+
expect(mkdirSync).toHaveBeenCalledWith(expect.any(String), {recursive: true})
160+
expect(writeJsonFileSync).toHaveBeenCalledWith(
161+
expect.any(String),
162+
{existing: 'value', newKey: 'newValue'},
163+
{pretty: true},
164+
)
165+
})
166+
167+
test('set overwrites existing key', () => {
168+
vi.mocked(readJsonFileSync).mockReturnValueOnce({
169+
myKey: 'oldValue',
170+
})
171+
172+
const store = getUserConfig()
173+
store.set('myKey', 'updatedValue')
174+
175+
expect(writeJsonFileSync).toHaveBeenCalledWith(
176+
expect.any(String),
177+
{myKey: 'updatedValue'},
178+
{pretty: true},
140179
)
141180
})
142181

143-
test('throws error for invalid value type', async () => {
144-
await expect(setCliUserConfig('authToken', 123 as never)).rejects.toThrow('Invalid value')
182+
test('set handles complex values', () => {
183+
vi.mocked(readJsonFileSync).mockReturnValueOnce({})
184+
185+
const store = getUserConfig()
186+
const complexValue = {status: 'granted', updatedAt: 12_345}
187+
store.set('telemetryConsent', complexValue)
188+
189+
expect(writeJsonFileSync).toHaveBeenCalledWith(
190+
expect.any(String),
191+
{telemetryConsent: complexValue},
192+
{pretty: true},
193+
)
145194
})
146195

147-
test('merges new config with existing config', async () => {
148-
vi.mocked(readJsonFile).mockResolvedValueOnce({
149-
authToken: 'existing-token',
196+
test('delete removes key from file and writes', () => {
197+
vi.mocked(readJsonFileSync).mockReturnValueOnce({
198+
keepMe: 'yes',
199+
removeMe: 'bye',
150200
})
151201

152-
await setCliUserConfig('telemetryConsent', {
153-
updatedAt: 123,
154-
value: {
155-
status: 'granted' as const,
156-
type: 'explicit',
157-
},
202+
const store = getUserConfig()
203+
store.delete('removeMe')
204+
205+
expect(mkdirSync).toHaveBeenCalledWith(expect.any(String), {recursive: true})
206+
expect(writeJsonFileSync).toHaveBeenCalledWith(
207+
expect.any(String),
208+
{keepMe: 'yes'},
209+
{pretty: true},
210+
)
211+
})
212+
213+
test('delete is a no-op write when key does not exist', () => {
214+
vi.mocked(readJsonFileSync).mockReturnValueOnce({
215+
existing: 'value',
158216
})
159217

160-
expect(writeJsonFile).toHaveBeenCalledWith(
218+
const store = getUserConfig()
219+
store.delete('nonexistent')
220+
221+
expect(writeJsonFileSync).toHaveBeenCalledWith(
161222
expect.any(String),
162-
expect.objectContaining({
163-
authToken: 'existing-token',
164-
telemetryConsent: expect.any(Object),
165-
}),
166-
expect.any(Object),
223+
{existing: 'value'},
224+
{pretty: true},
167225
)
168226
})
227+
228+
test('each operation does a fresh read', () => {
229+
const store = getUserConfig()
230+
231+
// First call returns one state
232+
vi.mocked(readJsonFileSync).mockReturnValueOnce({counter: 1})
233+
expect(store.get('counter')).toBe(1)
234+
235+
// Second call returns updated state
236+
vi.mocked(readJsonFileSync).mockReturnValueOnce({counter: 2})
237+
expect(store.get('counter')).toBe(2)
238+
239+
expect(readJsonFileSync).toHaveBeenCalledTimes(2)
240+
})
169241
})
170242
})

packages/@sanity/cli-core/src/index.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ export * from './SanityCommand.js'
2020
export * from './services/apiClient.js'
2121
export * from './services/cliUserConfig.js'
2222
export * from './services/getCliToken.js'
23-
export * from './telemetry/getTelemetryBaseInfo.js'
23+
export {getTelemetryBaseInfo} from './telemetry/getTelemetryBaseInfo.js'
2424
export {
2525
type CLITelemetryStore,
2626
type ConsentInformation,
@@ -34,10 +34,9 @@ export {
3434
CLI_TELEMETRY_SYMBOL,
3535
getCliTelemetry,
3636
setCliTelemetry,
37-
} from './util/getCliTelemetry.js'
37+
} from './telemetry/getCliTelemetry.js'
3838
export * from './util/getSanityEnvVar.js'
3939
export * from './util/getSanityUrl.js'
40-
export * from './util/getUserConfig.js'
4140
export * from './util/importModule.js'
4241
export * from './util/isCi.js'
4342
export * from './util/isInteractive.js'

0 commit comments

Comments
 (0)