Skip to content

Commit 47f6d8e

Browse files
salmanmkcsalmanmkc
authored
fix: rename binding to createOctokit and address review findings
- Rename context binding from getOctokit to createOctokit to avoid SyntaxError when users write const { getOctokit } = require(...) in their scripts (~10 public workflows affected) - Strip undefined values from user options to prevent clobbering defaults (e.g. GHES baseUrl) - Deep-merge retry options alongside request options - Use nullish coalescing (??) instead of logical OR (||) - Shallow-copy opts to prevent shared reference mutation - Add tests: undefined stripping, retry merge, falsy value preservation, no mutation of defaults - 32 tests passing, lint clean, dist rebuilt
1 parent 95933be commit 47f6d8e

8 files changed

Lines changed: 169 additions & 63 deletions

‎__test__/async-function.test.ts‎

Lines changed: 30 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -8,26 +8,28 @@ describe('callAsyncFunction', () => {
88
expect(result).toEqual('bar')
99
})
1010

11-
test('passes getOctokit through the script context', async () => {
12-
const getOctokit = jest.fn().mockReturnValue('secondary-client')
11+
test('passes createOctokit through the script context', async () => {
12+
const createOctokit = jest.fn().mockReturnValue('secondary-client')
1313

1414
const result = await callAsyncFunction(
15-
{getOctokit} as any,
16-
"return getOctokit('token')"
15+
{createOctokit} as any,
16+
"return createOctokit('token')"
1717
)
1818

19-
expect(getOctokit).toHaveBeenCalledWith('token')
19+
expect(createOctokit).toHaveBeenCalledWith('token')
2020
expect(result).toEqual('secondary-client')
2121
})
2222

23-
test('getOctokit creates client independent from github', async () => {
23+
test('createOctokit creates client independent from github', async () => {
2424
const github = {rest: {issues: 'primary'}}
25-
const getOctokit = jest.fn().mockReturnValue({rest: {issues: 'secondary'}})
25+
const createOctokit = jest
26+
.fn()
27+
.mockReturnValue({rest: {issues: 'secondary'}})
2628

2729
const result = await callAsyncFunction(
28-
{github, getOctokit} as any,
30+
{github, createOctokit} as any,
2931
`
30-
const secondary = getOctokit('other-token')
32+
const secondary = createOctokit('other-token')
3133
return {
3234
primary: github.rest.issues,
3335
secondary: secondary.rest.issues,
@@ -41,32 +43,32 @@ describe('callAsyncFunction', () => {
4143
secondary: 'secondary',
4244
different: true
4345
})
44-
expect(getOctokit).toHaveBeenCalledWith('other-token')
46+
expect(createOctokit).toHaveBeenCalledWith('other-token')
4547
})
4648

47-
test('getOctokit passes options through', async () => {
48-
const getOctokit = jest.fn().mockReturnValue('client-with-opts')
49+
test('createOctokit passes options through', async () => {
50+
const createOctokit = jest.fn().mockReturnValue('client-with-opts')
4951

5052
const result = await callAsyncFunction(
51-
{getOctokit} as any,
52-
`return getOctokit('my-token', { baseUrl: 'https://ghes.example.com/api/v3' })`
53+
{createOctokit} as any,
54+
`return createOctokit('my-token', { baseUrl: 'https://ghes.example.com/api/v3' })`
5355
)
5456

55-
expect(getOctokit).toHaveBeenCalledWith('my-token', {
57+
expect(createOctokit).toHaveBeenCalledWith('my-token', {
5658
baseUrl: 'https://ghes.example.com/api/v3'
5759
})
5860
expect(result).toEqual('client-with-opts')
5961
})
6062

61-
test('getOctokit supports plugins', async () => {
62-
const getOctokit = jest.fn().mockReturnValue('client-with-plugins')
63+
test('createOctokit supports plugins', async () => {
64+
const createOctokit = jest.fn().mockReturnValue('client-with-plugins')
6365

6466
const result = await callAsyncFunction(
65-
{getOctokit} as any,
66-
`return getOctokit('my-token', { previews: ['v3'] }, 'pluginA', 'pluginB')`
67+
{createOctokit} as any,
68+
`return createOctokit('my-token', { previews: ['v3'] }, 'pluginA', 'pluginB')`
6769
)
6870

69-
expect(getOctokit).toHaveBeenCalledWith(
71+
expect(createOctokit).toHaveBeenCalledWith(
7072
'my-token',
7173
{previews: ['v3']},
7274
'pluginA',
@@ -75,24 +77,24 @@ describe('callAsyncFunction', () => {
7577
expect(result).toEqual('client-with-plugins')
7678
})
7779

78-
test('multiple getOctokit calls produce independent clients', async () => {
79-
const getOctokit = jest
80+
test('multiple createOctokit calls produce independent clients', async () => {
81+
const createOctokit = jest
8082
.fn()
8183
.mockReturnValueOnce({id: 'client-a'})
8284
.mockReturnValueOnce({id: 'client-b'})
8385

8486
const result = await callAsyncFunction(
85-
{getOctokit} as any,
87+
{createOctokit} as any,
8688
`
87-
const a = getOctokit('token-a')
88-
const b = getOctokit('token-b')
89+
const a = createOctokit('token-a')
90+
const b = createOctokit('token-b')
8991
return { a: a.id, b: b.id, different: a !== b }
9092
`
9193
)
9294

93-
expect(getOctokit).toHaveBeenCalledTimes(2)
94-
expect(getOctokit).toHaveBeenNthCalledWith(1, 'token-a')
95-
expect(getOctokit).toHaveBeenNthCalledWith(2, 'token-b')
95+
expect(createOctokit).toHaveBeenCalledTimes(2)
96+
expect(createOctokit).toHaveBeenNthCalledWith(1, 'token-a')
97+
expect(createOctokit).toHaveBeenNthCalledWith(2, 'token-b')
9698
expect(result).toEqual({a: 'client-a', b: 'client-b', different: true})
9799
})
98100

‎__test__/create-configured-getoctokit.test.ts‎

Lines changed: 76 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,23 @@ describe('createConfiguredGetOctokit', () => {
7676
)
7777
})
7878

79+
test('deep-merges retry so partial overrides preserve existing settings', () => {
80+
const raw = makeMockGetOctokit()
81+
const defaults = {
82+
retry: {enabled: true, retries: 3}
83+
}
84+
85+
const wrapped = createConfiguredGetOctokit(raw as any, defaults)
86+
wrapped('tok' as any, {retry: {retries: 5}} as any)
87+
88+
expect(raw).toHaveBeenCalledWith(
89+
'tok',
90+
expect.objectContaining({
91+
retry: {enabled: true, retries: 5}
92+
})
93+
)
94+
})
95+
7996
test('user can override request.retries explicitly', () => {
8097
const raw = makeMockGetOctokit()
8198
const defaults = {request: {retries: 3}}
@@ -158,15 +175,35 @@ describe('createConfiguredGetOctokit', () => {
158175

159176
test('baseUrl: undefined from user does not clobber default', () => {
160177
const raw = makeMockGetOctokit()
161-
const defaults = {baseUrl: 'https://api.github.com'}
178+
const defaults = {baseUrl: 'https://ghes.example.com/api/v3'}
162179

163180
const wrapped = createConfiguredGetOctokit(raw as any, defaults)
164181
wrapped('tok' as any, {baseUrl: undefined} as any)
165182

166-
// undefined spread still overwrites — this documents current behavior.
167-
// The `baseUrl` key is present but value is undefined.
168183
const calledOpts = raw.mock.calls[0][1]
169-
expect(calledOpts).toHaveProperty('baseUrl')
184+
expect(calledOpts.baseUrl).toBe('https://ghes.example.com/api/v3')
185+
})
186+
187+
test('undefined values in nested request are stripped', () => {
188+
const raw = makeMockGetOctokit()
189+
const defaults = {request: {retries: 3, agent: 'proxy'}}
190+
191+
const wrapped = createConfiguredGetOctokit(raw as any, defaults)
192+
wrapped('tok' as any, {request: {retries: undefined, timeout: 5000}} as any)
193+
194+
const calledOpts = raw.mock.calls[0][1]
195+
expect(calledOpts.request).toEqual({retries: 3, agent: 'proxy', timeout: 5000})
196+
})
197+
198+
test('undefined values in nested retry are stripped', () => {
199+
const raw = makeMockGetOctokit()
200+
const defaults = {retry: {enabled: true, retries: 3}}
201+
202+
const wrapped = createConfiguredGetOctokit(raw as any, defaults)
203+
wrapped('tok' as any, {retry: {enabled: undefined, retries: 5}} as any)
204+
205+
const calledOpts = raw.mock.calls[0][1]
206+
expect(calledOpts.retry).toEqual({enabled: true, retries: 5})
170207
})
171208

172209
test('each call creates an independent client', () => {
@@ -183,4 +220,39 @@ describe('createConfiguredGetOctokit', () => {
183220
expect(b).toBe('client-b')
184221
expect(raw).toHaveBeenCalledTimes(2)
185222
})
223+
224+
test('does not mutate defaultOptions between calls', () => {
225+
const raw = makeMockGetOctokit()
226+
const defaults = {
227+
request: {retries: 3},
228+
retry: {enabled: true}
229+
}
230+
const originalDefaults = JSON.parse(JSON.stringify(defaults))
231+
232+
const wrapped = createConfiguredGetOctokit(raw as any, defaults)
233+
wrapped('tok' as any, {request: {timeout: 5000}, retry: {retries: 10}} as any)
234+
wrapped('tok' as any, {request: {timeout: 9000}} as any)
235+
236+
expect(defaults).toEqual(originalDefaults)
237+
})
238+
239+
test('falsy-but-valid values are preserved, only undefined is stripped', () => {
240+
const raw = makeMockGetOctokit()
241+
const defaults = {baseUrl: 'https://ghes.example.com/api/v3'}
242+
243+
const wrapped = createConfiguredGetOctokit(raw as any, defaults)
244+
wrapped('tok' as any, {
245+
log: null,
246+
retries: 0,
247+
debug: false,
248+
userAgent: ''
249+
} as any)
250+
251+
const calledOpts = raw.mock.calls[0][1]
252+
expect(calledOpts.log).toBeNull()
253+
expect(calledOpts.retries).toBe(0)
254+
expect(calledOpts.debug).toBe(false)
255+
expect(calledOpts.userAgent).toBe('')
256+
expect(calledOpts.baseUrl).toBe('https://ghes.example.com/api/v3')
257+
})
186258
})

‎__test__/getoctokit-integration.test.ts‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,12 @@
33
import {getOctokit} from '@actions/github'
44
import {callAsyncFunction} from '../src/async-function'
55

6-
describe('getOctokit integration via callAsyncFunction', () => {
6+
describe('createOctokit integration via callAsyncFunction', () => {
77
test('real getOctokit creates a functional Octokit client in script scope', async () => {
88
const result = await callAsyncFunction(
9-
{getOctokit} as any,
9+
{createOctokit: getOctokit} as any,
1010
`
11-
const client = getOctokit('fake-token-for-test')
11+
const client = createOctokit('fake-token-for-test')
1212
return {
1313
hasRest: typeof client.rest === 'object',
1414
hasGraphql: typeof client.graphql === 'function',
@@ -32,9 +32,9 @@ describe('getOctokit integration via callAsyncFunction', () => {
3232
const primary = getOctokit('primary-token')
3333

3434
const result = await callAsyncFunction(
35-
{github: primary, getOctokit} as any,
35+
{github: primary, createOctokit: getOctokit} as any,
3636
`
37-
const secondary = getOctokit('secondary-token')
37+
const secondary = createOctokit('secondary-token')
3838
return {
3939
bothHaveRest: typeof github.rest === 'object' && typeof secondary.rest === 'object',
4040
areDistinct: github !== secondary
@@ -48,11 +48,11 @@ describe('getOctokit integration via callAsyncFunction', () => {
4848
})
4949
})
5050

51-
test('getOctokit accepts options for GHES base URL', async () => {
51+
test('createOctokit accepts options for GHES base URL', async () => {
5252
const result = await callAsyncFunction(
53-
{getOctokit} as any,
53+
{createOctokit: getOctokit} as any,
5454
`
55-
const client = getOctokit('fake-token', {
55+
const client = createOctokit('fake-token', {
5656
baseUrl: 'https://ghes.example.com/api/v3'
5757
})
5858
return typeof client.rest === 'object'
@@ -62,12 +62,12 @@ describe('getOctokit integration via callAsyncFunction', () => {
6262
expect(result).toBe(true)
6363
})
6464

65-
test('multiple getOctokit calls produce independent clients with different tokens', async () => {
65+
test('multiple createOctokit calls produce independent clients with different tokens', async () => {
6666
const result = await callAsyncFunction(
67-
{getOctokit} as any,
67+
{createOctokit: getOctokit} as any,
6868
`
69-
const clientA = getOctokit('token-a')
70-
const clientB = getOctokit('token-b')
69+
const clientA = createOctokit('token-a')
70+
const clientB = createOctokit('token-b')
7171
return {
7272
aHasRest: typeof clientA.rest === 'object',
7373
bHasRest: typeof clientB.rest === 'object',

‎dist/index.js‎

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -36189,29 +36189,40 @@ function callAsyncFunction(args, source) {
3618936189
}
3619036190

3619136191
;// CONCATENATED MODULE: ./src/create-configured-getoctokit.ts
36192+
/**
36193+
* Strip keys whose value is `undefined` so they don't clobber defaults
36194+
* during object spread (e.g. `{baseUrl: undefined}` would wipe a GHES URL).
36195+
*/
36196+
function stripUndefined(obj) {
36197+
return Object.fromEntries(Object.entries(obj).filter(([, v]) => v !== undefined));
36198+
}
3619236199
/**
3619336200
* Creates a wrapped getOctokit that inherits default options and plugins.
3619436201
* Secondary clients created via the wrapper get the same retry, logging,
3619536202
* orchestration ID, and retries count as the pre-built `github` client.
3619636203
*
36197-
* - `request` is deep-merged so partial overrides (e.g. `{request: {timeout: 5000}}`)
36198-
* don't clobber inherited `retries`, proxy agent, or fetch defaults.
36204+
* - `request` and `retry` are deep-merged so partial overrides
36205+
* (e.g. `{request: {timeout: 5000}}`) don't clobber inherited values.
36206+
* - `undefined` values in user options are stripped to prevent accidental
36207+
* clobbering of defaults (e.g. GHES `baseUrl`).
3619936208
* - Default plugins (retry, requestLog) are always included; duplicates are skipped.
3620036209
*/
3620136210
function createConfiguredGetOctokit(rawGetOctokit, defaultOptions,
3620236211
// eslint-disable-next-line @typescript-eslint/no-explicit-any
3620336212
...defaultPlugins) {
3620436213
// eslint-disable-next-line @typescript-eslint/no-explicit-any
3620536214
return ((token, options, ...plugins) => {
36206-
var _a, _b;
36207-
const userOpts = options || {};
36215+
var _a, _b, _c, _d;
36216+
const userOpts = stripUndefined(options !== null && options !== void 0 ? options : {});
3620836217
const defaultRequest = (_a = defaultOptions.request) !== null && _a !== void 0 ? _a : {};
36209-
const userRequest = (_b = userOpts.request) !== null && _b !== void 0 ? _b : {};
36218+
const userRequest = stripUndefined((_b = userOpts.request) !== null && _b !== void 0 ? _b : {});
36219+
const defaultRetry = (_c = defaultOptions.retry) !== null && _c !== void 0 ? _c : {};
36220+
const userRetry = stripUndefined((_d = userOpts.retry) !== null && _d !== void 0 ? _d : {});
3621036221
const merged = {
3621136222
...defaultOptions,
3621236223
...userOpts,
36213-
// Deep-merge `request` to preserve retries, proxy agent, and fetch
36214-
request: { ...defaultRequest, ...userRequest }
36224+
request: { ...defaultRequest, ...userRequest },
36225+
retry: { ...defaultRetry, ...userRetry }
3621536226
};
3621636227
// Deduplicate: default plugins first, then user plugins that aren't already present
3621736228
const allPlugins = [...defaultPlugins];
@@ -36322,14 +36333,14 @@ async function main() {
3632236333
const script = core.getInput('script', { required: true });
3632336334
// Wrap getOctokit so secondary clients inherit retry, logging,
3632436335
// orchestration ID, and the action's retries input.
36325-
const configuredGetOctokit = createConfiguredGetOctokit(lib_github.getOctokit, opts, plugin_retry_dist_node.retry, dist_node.requestLog);
36336+
const configuredGetOctokit = createConfiguredGetOctokit(lib_github.getOctokit, { ...opts }, plugin_retry_dist_node.retry, dist_node.requestLog);
3632636337
// Using property/value shorthand on `require` (e.g. `{require}`) causes compilation errors.
3632736338
const result = await callAsyncFunction({
3632836339
require: wrapRequire,
3632936340
__original_require__: require,
3633036341
github,
3633136342
octokit: github,
36332-
getOctokit: configuredGetOctokit,
36343+
createOctokit: configuredGetOctokit,
3633336344
context: lib_github.context,
3633436345
core: core,
3633536346
exec: exec,

‎src/async-function.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ export declare type AsyncFunctionArguments = {
1313
core: typeof core
1414
github: InstanceType<typeof GitHub>
1515
octokit: InstanceType<typeof GitHub>
16-
getOctokit: typeof getOctokit
16+
createOctokit: typeof getOctokit
1717
exec: typeof exec
1818
glob: typeof glob
1919
io: typeof io

0 commit comments

Comments
 (0)