Skip to content
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

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

Merged
merged 17 commits into from
Sep 11, 2023
6 changes: 6 additions & 0 deletions docs/content/contributing/guidelines-frontend.en-us.md
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,12 @@ it's recommended to use `const _promise = asyncFoo()` to tell readers
that this is done by purpose, we want to call the async function and ignore the Promise.
Some lint rules and IDEs also have warnings if the returned Promise is not handled.

### Fetching data

To fetch data, use the wrapper functions `GET`, `POST` etc. from `modules/fetch.js`. They
accept a `data` option for the content, will automatically set CSFR token and return a
Copy link
Member

Choose a reason for hiding this comment

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

CSRF

Copy link
Member Author

Choose a reason for hiding this comment

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

Will fix this in followup #27026.

Promise for a [Response](https://developer.mozilla.org/en-US/docs/Web/API/Response).

### HTML Attributes and `dataset`

The usage of `dataset` is forbidden, its camel-casing behaviour makes it hard to grep for attributes.
Expand Down
52 changes: 24 additions & 28 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} 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 @@ -196,17 +195,21 @@ const sfc = {
}
},

async fetchArtifacts() {
const resp = await POST(`${this.actionsURL}/runs/${this.runIndex}/artifacts`);
return await resp.json();
},

async fetchJob() {
const logCursors = this.currentJobStepsStates.map((it, idx) => {
// cursor is used to indicate the last position of the logs
// it's only used by backend, frontend just reads it and passes it back, it and can be any type.
// 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}`, {
data: {logCursors},
});
return await resp.json();
},

Expand All @@ -215,16 +218,21 @@ const sfc = {
try {
this.loading = true;

// refresh artifacts if upload-artifact step done
const resp = await this.fetchPost(`${this.actionsURL}/runs/${this.runIndex}/artifacts`);
const artifacts = await resp.json();
this.artifacts = artifacts['artifacts'] || [];
let job, artifacts;
try {
[job, artifacts] = await Promise.all([
this.fetchJob(),
this.fetchArtifacts(), // refresh artifacts if upload-artifact step done
]);
} catch (err) {
if (!(err instanceof TypeError)) throw err; // avoid network error while unloading page
}

const response = await this.fetchJob();
this.artifacts = artifacts['artifacts'] || [];

// save the state to Vue data, then the UI will be updated
this.run = response.state.run;
this.currentJob = response.state.currentJob;
this.run = job.state.run;
this.currentJob = job.state.currentJob;

// sync the currentJobStepsStates to store the job step states
for (let i = 0; i < this.currentJob.steps.length; i++) {
Expand All @@ -234,7 +242,7 @@ const sfc = {
}
}
// append logs to the UI
for (const logs of response.logs.stepsLog) {
for (const logs of job.logs.stepsLog) {
// save the cursor, it will be passed to backend next time
this.currentJobStepsStates[logs.step].cursor = logs.cursor;
this.appendLogs(logs.step, logs.lines, logs.started);
Expand All @@ -249,18 +257,6 @@ const sfc = {
}
},


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
38 changes: 38 additions & 0 deletions web_src/js/modules/fetch.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import {isObject} from '../utils.js';

const {csrfToken} = window.config;

// fetch wrapper, use below method name functions and the `data` option to pass in data
// which will automatically set an appropriate content-type header. For json content,
// only object and array types are currently supported.
function request(url, {headers, data, body, ...other} = {}) {
let contentType;
if (!body) {
if (data instanceof FormData) {
contentType = 'multipart/form-data';
body = data;
} else if (data instanceof URLSearchParams) {
contentType = 'application/x-www-form-urlencoded';
Copy link
Contributor

@wxiaoguang wxiaoguang Sep 10, 2023

Choose a reason for hiding this comment

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

multipart/form-data could be always the correct content type (well, without a file, application/x-www-form-urlencoded might be simpler)

While I do not see why it should support URLSearchParams, the URLSearchParams should mainly used for URL query string, but not a POST request.

(not blocker)

Copy link
Member Author

@silverwind silverwind Sep 11, 2023

Choose a reason for hiding this comment

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

Does the backend actually support both encodings interchangbly? Normal form behaviour is application/x-www-form-urlencoded for forms not containing a file, and I think the cleanest way for these is to encode them as URLSearchParams which ensures escaping is correct etc:

https://stackoverflow.com/questions/35325370/how-do-i-post-a-x-www-form-urlencoded-request-using-fetch

body = data;
} else if (isObject(data) || Array.isArray(data)) {
contentType = 'application/json';
body = JSON.stringify(data);
}
}

return 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

...(contentType && {'content-type': contentType}),
...headers,
},
...(body && {body}),
...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});
11 changes: 11 additions & 0 deletions web_src/js/modules/fetch.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import {test, expect} from 'vitest';
import {GET, POST, PATCH, PUT, DELETE} from './fetch.js';

// tests here are only to satisfy the linter for unused functions
test('exports', () => {
expect(GET).toBeTruthy();
expect(POST).toBeTruthy();
expect(PATCH).toBeTruthy();
expect(PUT).toBeTruthy();
expect(DELETE).toBeTruthy();
});
silverwind marked this conversation as resolved.
Show resolved Hide resolved