Skip to content

Commit 0e657c6

Browse files
kanadguptakanadgupta
authored
fix(docs/upload): switch from HEAD to GET for initial page fetch (#1224)
| 🚥 Resolves RM-11904 | | :------------------- | ## 🧰 Changes when making `HEAD` requests, there is no response body, meaning that errors are totally unhelpful. this switches our initial page fetch requests to use the `GET` method.
1 parent 52889b3 commit 0e657c6

File tree

4 files changed

+108
-52
lines changed

4 files changed

+108
-52
lines changed

‎__tests__/commands/docs/__snapshots__/upload.test.ts.snap‎

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -320,9 +320,27 @@ exports[`rdme docs upload > given that the file path is a single file > and the
320320
}
321321
`;
322322
323-
exports[`rdme docs upload > given that the file path is a single file > given that the --dry-run flag is passed > should error out if a non-404 error is returned from the HEAD request 1`] = `
323+
exports[`rdme docs upload > given that the file path is a single file > given that the --dry-run flag is passed > should error out if a non-404 error is returned from the GET request (with a json body) 1`] = `
324324
{
325-
"error": [APIv2Error: The ReadMe API responded with an unexpected error. Please try again and if this issue persists, get in touch with us at support@readme.io.],
325+
"error": [APIv2Error: ReadMe API error: bad request
326+
327+
something went so so wrong],
328+
"stderr": " › Warning: This command is in an experimental alpha and is likely to change.
329+
› Use at your own risk!
330+
- 🔬 Validating frontmatter data...
331+
✔ 🔬 Validating frontmatter data... no issues found!
332+
- 🎭 Uploading files to ReadMe (but not really because it's a dry run)...
333+
✖ 🎭 Uploading files to ReadMe (but not really because it's a dry run)... 1 file(s) failed.
334+
",
335+
"stdout": "🚨 Unable to fetch data about the following 1 page(s):
336+
- __tests__/__fixtures__/docs/slug-docs/new-doc-slug.md: bad request
337+
",
338+
}
339+
`;
340+
341+
exports[`rdme docs upload > given that the file path is a single file > given that the --dry-run flag is passed > should error out if a non-404 error is returned from the GET request 1`] = `
342+
{
343+
"error": [Error: The ReadMe API responded with an unexpected error. Please try again and if this issue persists, get in touch with us at support@readme.io.],
326344
"stderr": " › Warning: This command is in an experimental alpha and is likely to change.
327345
› Use at your own risk!
328346
- 🔬 Validating frontmatter data...
@@ -621,9 +639,27 @@ exports[`rdme docs upload > given that the file path is a single file > should c
621639
}
622640
`;
623641
624-
exports[`rdme docs upload > given that the file path is a single file > should error out if a non-404 error is returned from the HEAD request 1`] = `
642+
exports[`rdme docs upload > given that the file path is a single file > should error out if a non-404 error is returned from the GET request (with a json body) 1`] = `
643+
{
644+
"error": [APIv2Error: ReadMe API error: bad request
645+
646+
something went so so wrong],
647+
"stderr": " › Warning: This command is in an experimental alpha and is likely to change.
648+
› Use at your own risk!
649+
- 🔬 Validating frontmatter data...
650+
✔ 🔬 Validating frontmatter data... no issues found!
651+
- 🚀 Uploading files to ReadMe...
652+
✖ 🚀 Uploading files to ReadMe... 1 file(s) failed.
653+
",
654+
"stdout": "🚨 Received errors when attempting to upload 1 page(s):
655+
- __tests__/__fixtures__/docs/new-docs/new-doc.md: bad request
656+
",
657+
}
658+
`;
659+
660+
exports[`rdme docs upload > given that the file path is a single file > should error out if a non-404 error is returned from the GET request 1`] = `
625661
{
626-
"error": [APIv2Error: The ReadMe API responded with an unexpected error. Please try again and if this issue persists, get in touch with us at support@readme.io.],
662+
"error": [Error: The ReadMe API responded with an unexpected error. Please try again and if this issue persists, get in touch with us at support@readme.io.],
627663
"stderr": " › Warning: This command is in an experimental alpha and is likely to change.
628664
› Use at your own risk!
629665
- 🔬 Validating frontmatter data...

‎__tests__/commands/docs/upload.test.ts‎

Lines changed: 64 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ describe('rdme docs upload', () => {
3737
describe('given that the file path is a single file', () => {
3838
it('should create a guides page in ReadMe', async () => {
3939
const mock = getAPIv2Mock({ authorization })
40-
.head('/versions/stable/guides/new-doc')
40+
.get('/versions/stable/guides/new-doc')
4141
.reply(404)
4242
.post('/versions/stable/guides', {
4343
category: { uri: '/versions/stable/categories/guides/category-slug' },
@@ -57,7 +57,7 @@ describe('rdme docs upload', () => {
5757

5858
it('should allow for user to specify version via --version flag', async () => {
5959
const mock = getAPIv2Mock({ authorization })
60-
.head('/versions/1.2.3/guides/new-doc')
60+
.get('/versions/1.2.3/guides/new-doc')
6161
.reply(404)
6262
.post('/versions/1.2.3/guides', {
6363
category: { uri: '/versions/1.2.3/categories/guides/category-slug' },
@@ -77,7 +77,7 @@ describe('rdme docs upload', () => {
7777

7878
describe('given that the --dry-run flag is passed', () => {
7979
it('should not create anything in ReadMe', async () => {
80-
const mock = getAPIv2Mock({ authorization }).head('/versions/stable/guides/new-doc').reply(404);
80+
const mock = getAPIv2Mock({ authorization }).get('/versions/stable/guides/new-doc').reply(404);
8181

8282
const result = await run(['__tests__/__fixtures__/docs/new-docs/new-doc.md', '--key', key, '--dry-run']);
8383

@@ -88,7 +88,7 @@ describe('rdme docs upload', () => {
8888
});
8989

9090
it('should not update anything in ReadMe', async () => {
91-
const mock = getAPIv2Mock({ authorization }).head('/versions/stable/guides/some-slug').reply(200);
91+
const mock = getAPIv2Mock({ authorization }).get('/versions/stable/guides/some-slug').reply(200);
9292

9393
const result = await run(['__tests__/__fixtures__/docs/slug-docs/new-doc-slug.md', '--key', key, '--dry-run']);
9494

@@ -98,8 +98,23 @@ describe('rdme docs upload', () => {
9898
mock.done();
9999
});
100100

101-
it('should error out if a non-404 error is returned from the HEAD request', async () => {
102-
const mock = getAPIv2Mock({ authorization }).head('/versions/stable/guides/some-slug').reply(500);
101+
it('should error out if a non-404 error is returned from the GET request', async () => {
102+
const mock = getAPIv2Mock({ authorization }).get('/versions/stable/guides/some-slug').reply(500);
103+
104+
const result = await run(['__tests__/__fixtures__/docs/slug-docs/new-doc-slug.md', '--key', key, '--dry-run']);
105+
106+
expect(result).toMatchSnapshot();
107+
expect(fs.writeFileSync).not.toHaveBeenCalled();
108+
109+
mock.done();
110+
});
111+
112+
it('should error out if a non-404 error is returned from the GET request (with a json body)', async () => {
113+
const mock = getAPIv2Mock({ authorization }).get('/versions/stable/guides/some-slug').reply(500, {
114+
title: 'bad request',
115+
detail: 'something went so so wrong',
116+
status: 500,
117+
});
103118

104119
const result = await run(['__tests__/__fixtures__/docs/slug-docs/new-doc-slug.md', '--key', key, '--dry-run']);
105120

@@ -113,7 +128,7 @@ describe('rdme docs upload', () => {
113128
describe('given that the slug is passed in the frontmatter', () => {
114129
it('should use that slug to create a page in ReadMe', async () => {
115130
const mock = getAPIv2Mock({ authorization })
116-
.head('/versions/stable/guides/some-slug')
131+
.get('/versions/stable/guides/some-slug')
117132
.reply(404)
118133
.post('/versions/stable/guides', {
119134
category: { uri: '/versions/stable/categories/guides/some-category-uri' },
@@ -133,7 +148,7 @@ describe('rdme docs upload', () => {
133148

134149
it('should use that slug update an existing guides page in ReadMe', async () => {
135150
const mock = getAPIv2Mock({ authorization })
136-
.head('/versions/stable/guides/some-slug')
151+
.get('/versions/stable/guides/some-slug')
137152
.reply(200)
138153
.patch('/versions/stable/guides/some-slug', {
139154
category: { uri: '/versions/stable/categories/guides/some-category-uri' },
@@ -154,7 +169,7 @@ describe('rdme docs upload', () => {
154169
describe('given that the file has frontmatter issues', () => {
155170
it('should fix the frontmatter issues in the file and create the corrected file in ReadMe', async () => {
156171
const mock = getAPIv2Mock({ authorization })
157-
.head('/versions/stable/guides/legacy-category')
172+
.get('/versions/stable/guides/legacy-category')
158173
.reply(404)
159174
.post('/versions/stable/guides', {
160175
category: { uri: '/versions/stable/categories/guides/uri-that-does-not-map-to-5ae122e10fdf4e39bb34db6f' },
@@ -188,7 +203,7 @@ describe('rdme docs upload', () => {
188203
});
189204

190205
const mock = getAPIv2Mock({ authorization })
191-
.head('/versions/stable/guides/legacy-category')
206+
.get('/versions/stable/guides/legacy-category')
192207
.reply(404)
193208
.post('/versions/stable/guides', {
194209
category: { uri: '/versions/stable/categories/guides/mapped-uri' },
@@ -229,7 +244,7 @@ describe('rdme docs upload', () => {
229244

230245
it('should skip client-side validation if the --skip-validation flag is passed', async () => {
231246
const mock = getAPIv2Mock({ authorization })
232-
.head('/versions/stable/guides/legacy-category')
247+
.get('/versions/stable/guides/legacy-category')
233248
.reply(404)
234249
.post('/versions/stable/guides', {
235250
category: '5ae122e10fdf4e39bb34db6f',
@@ -254,7 +269,7 @@ describe('rdme docs upload', () => {
254269

255270
it('should warn user if the file has no autofixable issues', async () => {
256271
const mock = getAPIv2Mock({ authorization })
257-
.head('/versions/stable/guides/invalid-attributes')
272+
.get('/versions/stable/guides/invalid-attributes')
258273
.reply(404)
259274
.post('/versions/stable/guides', {
260275
category: {
@@ -282,7 +297,7 @@ describe('rdme docs upload', () => {
282297
afterEach(githubActionsEnv.after);
283298

284299
it('should create a guides page in ReadMe and include `x-readme-source-url` source header', async () => {
285-
const headMock = getAPIv2MockForGHA({ authorization }).head('/versions/stable/guides/new-doc').reply(404);
300+
const headMock = getAPIv2MockForGHA({ authorization }).get('/versions/stable/guides/new-doc').reply(404);
286301

287302
const postMock = getAPIv2MockForGHA({
288303
authorization,
@@ -323,8 +338,23 @@ describe('rdme docs upload', () => {
323338
});
324339
});
325340

326-
it('should error out if a non-404 error is returned from the HEAD request', async () => {
327-
const mock = getAPIv2Mock({ authorization }).head('/versions/stable/guides/new-doc').reply(500);
341+
it('should error out if a non-404 error is returned from the GET request', async () => {
342+
const mock = getAPIv2Mock({ authorization }).get('/versions/stable/guides/new-doc').reply(500);
343+
344+
const result = await run(['__tests__/__fixtures__/docs/new-docs/new-doc.md', '--key', key]);
345+
346+
expect(result).toMatchSnapshot();
347+
expect(fs.writeFileSync).not.toHaveBeenCalled();
348+
349+
mock.done();
350+
});
351+
352+
it('should error out if a non-404 error is returned from the GET request (with a json body)', async () => {
353+
const mock = getAPIv2Mock({ authorization }).get('/versions/stable/guides/new-doc').reply(500, {
354+
title: 'bad request',
355+
detail: 'something went so so wrong',
356+
status: 500,
357+
});
328358

329359
const result = await run(['__tests__/__fixtures__/docs/new-docs/new-doc.md', '--key', key]);
330360

@@ -352,15 +382,15 @@ describe('rdme docs upload', () => {
352382
describe('given that the file path is a directory', () => {
353383
it('should create a guides page in ReadMe for each file in the directory and its subdirectories', async () => {
354384
const mock = getAPIv2Mock({ authorization })
355-
.head('/versions/stable/guides/simple-doc')
385+
.get('/versions/stable/guides/simple-doc')
356386
.reply(404)
357387
.post('/versions/stable/guides', {
358388
slug: 'simple-doc',
359389
title: 'This is the document title',
360390
content: { body: '\nBody\n' },
361391
})
362392
.reply(201, {})
363-
.head('/versions/stable/guides/another-doc')
393+
.get('/versions/stable/guides/another-doc')
364394
.reply(404)
365395
.post('/versions/stable/guides', {
366396
slug: 'another-doc',
@@ -380,14 +410,14 @@ describe('rdme docs upload', () => {
380410

381411
it('should update existing guides pages in ReadMe for each file in the directory and its subdirectories', async () => {
382412
const mock = getAPIv2Mock({ authorization })
383-
.head('/versions/stable/guides/simple-doc')
413+
.get('/versions/stable/guides/simple-doc')
384414
.reply(200)
385415
.patch('/versions/stable/guides/simple-doc', {
386416
title: 'This is the document title',
387417
content: { body: '\nBody\n' },
388418
})
389419
.reply(201, {})
390-
.head('/versions/stable/guides/another-doc')
420+
.get('/versions/stable/guides/another-doc')
391421
.reply(200)
392422
.patch('/versions/stable/guides/another-doc', {
393423
title: 'This is another document title',
@@ -406,7 +436,7 @@ describe('rdme docs upload', () => {
406436

407437
it('should handle complex frontmatter', async () => {
408438
const mock = getAPIv2Mock({ authorization })
409-
.head('/versions/stable/guides/basic')
439+
.get('/versions/stable/guides/basic')
410440
.reply(200)
411441
.patch('/versions/stable/guides/basic', {
412442
title: 'This is the document title',
@@ -415,7 +445,7 @@ describe('rdme docs upload', () => {
415445
type: 'basic',
416446
})
417447
.reply(201, {})
418-
.head('/versions/stable/guides/link')
448+
.get('/versions/stable/guides/link')
419449
.reply(200)
420450
.patch('/versions/stable/guides/link', {
421451
title: 'This is the document title',
@@ -440,7 +470,7 @@ describe('rdme docs upload', () => {
440470
describe('given that the directory contains parent/child docs', () => {
441471
it('should upload parents before children', async () => {
442472
const mock = getAPIv2Mock({ authorization })
443-
.head('/versions/stable/guides/child')
473+
.get('/versions/stable/guides/child')
444474
.reply(404)
445475
.post('/versions/stable/guides', {
446476
slug: 'child',
@@ -449,15 +479,15 @@ describe('rdme docs upload', () => {
449479
content: { body: '\nBody\n' },
450480
})
451481
.reply(201, {})
452-
.head('/versions/stable/guides/friend')
482+
.get('/versions/stable/guides/friend')
453483
.reply(404)
454484
.post('/versions/stable/guides', {
455485
slug: 'friend',
456486
title: 'Friend',
457487
content: { body: '\nBody\n' },
458488
})
459489
.reply(201, {})
460-
.head('/versions/stable/guides/parent')
490+
.get('/versions/stable/guides/parent')
461491
.reply(404)
462492
.post('/versions/stable/guides', {
463493
slug: 'parent',
@@ -466,7 +496,7 @@ describe('rdme docs upload', () => {
466496
content: { body: '\nBody\n' },
467497
})
468498
.reply(201, {})
469-
.head('/versions/stable/guides/grandparent')
499+
.get('/versions/stable/guides/grandparent')
470500
.reply(404)
471501
.post('/versions/stable/guides', {
472502
slug: 'grandparent',
@@ -492,7 +522,7 @@ describe('rdme docs upload', () => {
492522

493523
it('should handle a mix of creates and updates and failures and skipped files', async () => {
494524
const mock = getAPIv2Mock({ authorization })
495-
.head('/versions/stable/guides/invalid-attributes')
525+
.get('/versions/stable/guides/invalid-attributes')
496526
.reply(404)
497527
.post('/versions/stable/guides', {
498528
category: { uri: '/versions/stable/categories/guides/some-category-uri', 'is-this-a-valid-property': 'nope' },
@@ -501,15 +531,15 @@ describe('rdme docs upload', () => {
501531
content: { body: '\nBody\n' },
502532
})
503533
.reply(201, {})
504-
.head('/versions/stable/guides/legacy-category')
534+
.get('/versions/stable/guides/legacy-category')
505535
.reply(200)
506536
.patch('/versions/stable/guides/legacy-category', {
507537
category: { uri: '/versions/stable/categories/guides/uri-that-does-not-map-to-5ae122e10fdf4e39bb34db6f' },
508538
title: 'This is the document title',
509539
content: { body: '\nBody\n' },
510540
})
511541
.reply(201, {})
512-
.head('/versions/stable/guides/some-slug')
542+
.get('/versions/stable/guides/some-slug')
513543
.reply(404)
514544
.post('/versions/stable/guides', {
515545
slug: 'some-slug',
@@ -518,7 +548,7 @@ describe('rdme docs upload', () => {
518548
content: { body: '\nBody\n' },
519549
})
520550
.reply(500, {})
521-
.head('/versions/stable/guides/simple-doc')
551+
.get('/versions/stable/guides/simple-doc')
522552
.reply(404)
523553
.post('/versions/stable/guides', {
524554
slug: 'simple-doc',
@@ -539,13 +569,13 @@ describe('rdme docs upload', () => {
539569

540570
it('should handle a mix of creates and updates and failures and skipped files (dry run)', async () => {
541571
const mock = getAPIv2Mock({ authorization })
542-
.head('/versions/stable/guides/invalid-attributes')
572+
.get('/versions/stable/guides/invalid-attributes')
543573
.reply(404)
544-
.head('/versions/stable/guides/legacy-category')
574+
.get('/versions/stable/guides/legacy-category')
545575
.reply(200)
546-
.head('/versions/stable/guides/some-slug')
576+
.get('/versions/stable/guides/some-slug')
547577
.reply(500)
548-
.head('/versions/stable/guides/simple-doc')
578+
.get('/versions/stable/guides/simple-doc')
549579
.reply(500);
550580

551581
prompts.inject([true]);
@@ -581,7 +611,7 @@ describe('rdme docs upload', () => {
581611
nock.cleanAll();
582612

583613
const mock = getAPIv2Mock({ authorization })
584-
.head('/versions/1.2.3/guides/new-doc')
614+
.get('/versions/1.2.3/guides/new-doc')
585615
.reply(404)
586616
.post('/versions/1.2.3/guides', {
587617
category: { uri: '/versions/1.2.3/categories/guides/category-slug' },

‎src/lib/readmeAPIFetch.ts‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -381,11 +381,6 @@ export async function handleAPIv2Res<R extends ResponseBody = ResponseBody, T ex
381381
*/
382382
this: T,
383383
res: Response,
384-
/**
385-
* If we're making a request where we don't care about the body (e.g. a HEAD or DELETE request),
386-
* we can skip parsing the JSON body using this flag.
387-
*/
388-
skipJsonParsing = false,
389384
): Promise<R> {
390385
const contentType = res.headers.get('content-type') || '';
391386
const extension = mime.extension(contentType) || contentType.includes('json') ? 'json' : false;
@@ -395,12 +390,6 @@ export async function handleAPIv2Res<R extends ResponseBody = ResponseBody, T ex
395390
const body = await res.text();
396391
this.debug(`received status code ${res.status} from ${res.url} with no content: ${body}`);
397392
return {} as R;
398-
} else if (skipJsonParsing) {
399-
// to prevent a memory leak, we should still consume the response body? even though we don't use it?
400-
// https://x.com/cramforce/status/1762142087930433999
401-
const body = await res.text();
402-
this.debug(`received status code ${res.status} from ${res.url} and not parsing JSON b/c of flag: ${body}`);
403-
return {} as R;
404393
} else if (extension === 'json') {
405394
const body = (await res.json()) as R;
406395
this.debug(`received status code ${res.status} from ${res.url} with JSON response: ${JSON.stringify(body)}`);

0 commit comments

Comments
 (0)