Skip to content

Add fetch wrappers, ignore network errors in actions view #26985

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Sep 11, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 12 additions & 21 deletions web_src/js/components/RepoActionView.vue
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@ import {createApp} from 'vue';
import {toggleElem} from '../utils/dom.js';
import {getCurrentLocale} from '../utils.js';
import {renderAnsi} from '../render/ansi.js';

const {csrfToken} = window.config;
import {POST, isNetworkError} from '../modules/fetch.js';

const sfc = {
name: 'RepoActionView',
Expand Down Expand Up @@ -145,11 +144,11 @@ const sfc = {
},
// cancel a run
cancelRun() {
this.fetchPost(`${this.run.link}/cancel`);
POST(`${this.run.link}/cancel`);
},
// approve a run
approveRun() {
this.fetchPost(`${this.run.link}/approve`);
POST(`${this.run.link}/approve`);
},

createLogLine(line, startTime, stepIndex) {
Expand Down Expand Up @@ -203,10 +202,9 @@ const sfc = {
// for example: make cursor=null means the first time to fetch logs, cursor=eof means no more logs, etc
return {step: idx, cursor: it.cursor, expanded: it.expanded};
});
const resp = await this.fetchPost(
`${this.actionsURL}/runs/${this.runIndex}/jobs/${this.jobIndex}`,
JSON.stringify({logCursors}),
);
const resp = await POST(`${this.actionsURL}/runs/${this.runIndex}/jobs/${this.jobIndex}`, {
json: {logCursors},
});
return await resp.json();
},

Expand All @@ -216,7 +214,7 @@ const sfc = {
this.loading = true;

// refresh artifacts if upload-artifact step done
const resp = await this.fetchPost(`${this.actionsURL}/runs/${this.runIndex}/artifacts`);
const resp = await POST(`${this.actionsURL}/runs/${this.runIndex}/artifacts`);
const artifacts = await resp.json();
this.artifacts = artifacts['artifacts'] || [];

Expand Down Expand Up @@ -244,23 +242,16 @@ const sfc = {
clearInterval(this.intervalID);
this.intervalID = null;
}
} catch (err) {
// avoid error while unloading page with fetch in progress
if (!isNetworkError(err.message)) {
throw err;
}
} finally {
this.loading = false;
}
},


fetchPost(url, body) {
return fetch(url, {
method: 'POST',
headers: {
'Content-Type': 'application/json',
'X-Csrf-Token': csrfToken,
},
body,
});
},

isDone(status) {
return ['success', 'skipped', 'failure', 'cancelled'].includes(status);
},
Expand Down
31 changes: 31 additions & 0 deletions web_src/js/modules/fetch.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
const {csrfToken} = window.config;

function request(url, {headers, json, ...other} = {}) {
return window.fetch(url, {
headers: {
'x-csrf-token': csrfToken,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are headers case-insensitive?
I've only seen them be called X-Csrf-Token yet…

Copy link
Member Author

@silverwind silverwind Sep 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, HTTP headers by definition are to be treated case-insensitive while reading them and in fact we should lowercase them all everywhere.

In HTTP2 the common headers are transferred via a mapping table that outputs lowercase: https://datatracker.ietf.org/doc/html/rfc7541#appendix-A

...(json !== undefined && {'content-type': 'application/json'}),
...headers,
},
...(json !== undefined && {body: JSON.stringify(json)}),
...other,
});
}

export const GET = (url, opts) => request(url, {method: 'GET', ...opts});
export const POST = (url, opts) => request(url, {method: 'POST', ...opts});
export const PATCH = (url, opts) => request(url, {method: 'PATCH', ...opts});
export const PUT = (url, opts) => request(url, {method: 'PUT', ...opts});
export const DELETE = (url, opts) => request(url, {method: 'DELETE', ...opts});

// network errors are currently only detectable by error message
// based on https://github.com/sindresorhus/p-retry/blob/main/index.js
const networkErrorMsgs = new Set([
'Failed to fetch', // Chrome
'NetworkError when attempting to fetch resource.', // Firefox
'The Internet connection appears to be offline.', // Safari
]);

export function isNetworkError(msg) {
return networkErrorMsgs.has(msg);
}
14 changes: 14 additions & 0 deletions web_src/js/modules/fetch.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import {test, expect} from 'vitest';
import {GET, POST, PATCH, PUT, DELETE, isNetworkError} from './fetch.js';

test('exports', () => {
expect(GET).toBeTruthy();
expect(POST).toBeTruthy();
expect(PATCH).toBeTruthy();
expect(PUT).toBeTruthy();
expect(DELETE).toBeTruthy();
});

test('isNetworkError', () => {
expect(isNetworkError('Failed to fetch')).toEqual(true);
});