Skip to content

Commit 996d94c

Browse files
committed
refactor(ts-sdk): remove misleading isBrowser() - SDK requires Node.js only
- Remove isBrowser() function that implied browser compatibility - Simplify getEnv() and isEnvSet() to directly use process.env - SDK fundamentally cannot run in browsers due to Node.js module imports - Add tests to verify isBrowser is not exported
1 parent 7588b77 commit 996d94c

File tree

3 files changed

+129
-18
lines changed

3 files changed

+129
-18
lines changed

rock/ts-sdk/src/index.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ export { RunMode, RunModeType, PID_PREFIX, PID_SUFFIX } from './common/constants
3434
export { HttpUtils } from './utils/http.js';
3535
export { retryAsync, sleep, withRetry } from './utils/retry.js';
3636
export { deprecated, deprecatedClass } from './utils/deprecated.js';
37-
export { isNode, isBrowser, getEnv, getRequiredEnv, isEnvSet } from './utils/system.js';
37+
export { isNode, getEnv, getRequiredEnv, isEnvSet } from './utils/system.js';
3838

3939
// EnvHub
4040
export * from './envhub/index.js';
Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
/**
2+
* Tests for system utilities
3+
*
4+
* These tests verify that the SDK explicitly requires Node.js environment
5+
* and does not include misleading browser compatibility checks.
6+
*/
7+
8+
import { isNode, getEnv, getRequiredEnv, isEnvSet } from './system.js';
9+
10+
describe('system utilities', () => {
11+
describe('isNode', () => {
12+
test('returns true in Node.js environment', () => {
13+
expect(isNode()).toBe(true);
14+
});
15+
});
16+
17+
describe('getEnv', () => {
18+
const originalEnv = process.env;
19+
20+
beforeEach(() => {
21+
// Reset process.env for each test
22+
process.env = { ...originalEnv };
23+
});
24+
25+
afterAll(() => {
26+
process.env = originalEnv;
27+
});
28+
29+
test('returns environment variable value when set', () => {
30+
process.env.TEST_VAR = 'test-value';
31+
expect(getEnv('TEST_VAR')).toBe('test-value');
32+
});
33+
34+
test('returns undefined when variable not set and no default', () => {
35+
delete process.env.NONEXISTENT_VAR;
36+
expect(getEnv('NONEXISTENT_VAR')).toBeUndefined();
37+
});
38+
39+
test('returns default value when variable not set', () => {
40+
delete process.env.NONEXISTENT_VAR;
41+
expect(getEnv('NONEXISTENT_VAR', 'default-value')).toBe('default-value');
42+
});
43+
44+
test('returns actual value even when default is provided', () => {
45+
process.env.TEST_VAR = 'actual-value';
46+
expect(getEnv('TEST_VAR', 'default-value')).toBe('actual-value');
47+
});
48+
});
49+
50+
describe('getRequiredEnv', () => {
51+
const originalEnv = process.env;
52+
53+
beforeEach(() => {
54+
process.env = { ...originalEnv };
55+
});
56+
57+
afterAll(() => {
58+
process.env = originalEnv;
59+
});
60+
61+
test('returns value when environment variable is set', () => {
62+
process.env.REQUIRED_VAR = 'required-value';
63+
expect(getRequiredEnv('REQUIRED_VAR')).toBe('required-value');
64+
});
65+
66+
test('throws error when environment variable is not set', () => {
67+
delete process.env.NONEXISTENT_REQUIRED_VAR;
68+
expect(() => getRequiredEnv('NONEXISTENT_REQUIRED_VAR')).toThrow(
69+
'Required environment variable NONEXISTENT_REQUIRED_VAR is not set'
70+
);
71+
});
72+
});
73+
74+
describe('isEnvSet', () => {
75+
const originalEnv = process.env;
76+
77+
beforeEach(() => {
78+
process.env = { ...originalEnv };
79+
});
80+
81+
afterAll(() => {
82+
process.env = originalEnv;
83+
});
84+
85+
test('returns true when environment variable is set', () => {
86+
process.env.SET_VAR = 'value';
87+
expect(isEnvSet('SET_VAR')).toBe(true);
88+
});
89+
90+
test('returns true when environment variable is set to empty string', () => {
91+
process.env.EMPTY_VAR = '';
92+
expect(isEnvSet('EMPTY_VAR')).toBe(true);
93+
});
94+
95+
test('returns false when environment variable is not set', () => {
96+
delete process.env.UNSET_VAR;
97+
expect(isEnvSet('UNSET_VAR')).toBe(false);
98+
});
99+
});
100+
});
101+
102+
/**
103+
* Test that isBrowser is NOT exported from the main index.
104+
* This SDK requires Node.js and should not have misleading browser checks.
105+
*/
106+
describe('Node.js only requirement', () => {
107+
test('isBrowser should not be exported from main index', async () => {
108+
const mainModule = await import('../index.js');
109+
110+
// isBrowser should NOT be exported - this is intentional
111+
expect('isBrowser' in mainModule).toBe(false);
112+
});
113+
114+
test('isBrowser should not be exported from system.js', async () => {
115+
// Re-import to check the module's exports
116+
const systemModule = await import('./system.js');
117+
118+
// isBrowser should NOT be exported
119+
expect('isBrowser' in systemModule).toBe(false);
120+
});
121+
});

rock/ts-sdk/src/utils/system.ts

Lines changed: 7 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
/**
22
* System utilities
3+
*
4+
* Note: This SDK requires Node.js environment (v20.8.0+).
5+
* It cannot run in browsers due to dependencies on Node.js modules.
36
*/
47

58
/**
@@ -13,23 +16,12 @@ export function isNode(): boolean {
1316
);
1417
}
1518

16-
/**
17-
* Check if running in browser environment
18-
*/
19-
export function isBrowser(): boolean {
20-
return typeof globalThis !== 'undefined' &&
21-
'window' in globalThis &&
22-
typeof (globalThis as Record<string, unknown>).window !== 'undefined';
23-
}
24-
2519
/**
2620
* Get environment variable
21+
* Directly accesses process.env since this SDK requires Node.js
2722
*/
2823
export function getEnv(key: string, defaultValue?: string): string | undefined {
29-
if (isNode()) {
30-
return process.env[key] ?? defaultValue;
31-
}
32-
return defaultValue;
24+
return process.env[key] ?? defaultValue;
3325
}
3426

3527
/**
@@ -45,10 +37,8 @@ export function getRequiredEnv(key: string): string {
4537

4638
/**
4739
* Check if environment variable is set
40+
* Directly checks process.env since this SDK requires Node.js
4841
*/
4942
export function isEnvSet(key: string): boolean {
50-
if (isNode()) {
51-
return key in process.env;
52-
}
53-
return false;
43+
return key in process.env;
5444
}

0 commit comments

Comments
 (0)