Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

feat: add support for conditional requests #119

Merged
merged 6 commits into from
Apr 2, 2019
Merged
Show file tree
Hide file tree
Changes from 5 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
6 changes: 6 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,12 @@
"typescript"
],
"jest": {
"globals": {
"caches": true
},
"setupFiles": [
"<rootDir>/test/setupTests.ts"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think ideally we'd fix build-config to resolve .ts setup files but this is okay for now.

],
"testPathIgnorePatterns": [
"<rootDir>/packages/generator-superset"
],
Expand Down
1 change: 1 addition & 0 deletions packages/superset-ui-connection/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ a high-level it supports:
- supports `GET` and `POST` requests (no `PUT` or `DELETE`)
- timeouts
- query aborts through the `AbortController` API
- conditional `GET` requests using `If-None-Match` and `ETag` headers
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd maybe mention Cache here? conditional sounds like something else to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Conditional request" is the actual name of a GET request with a If-None-Match header containing the ETag of the resource: https://developer.mozilla.org/en-US/docs/Web/HTTP/Conditional_requests#Conditional_headers. The fact that we're using the Cache API here is an implementation detail, we could use local storage, session storage or cookies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To expand on my comment: we're not simply caching, we're storing the resources and reusing them if and only if the hash hasn't changed.


#### Example usage

Expand Down
38 changes: 37 additions & 1 deletion packages/superset-ui-connection/src/callApi/callApi.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
/* eslint compat/compat: 'off' */
import 'whatwg-fetch';
import { CallApi } from '../types';
import { CACHE_KEY, NOT_MODIFIED, OK } from '../constants';
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry only nit with the status constants is that there's no context if you read them in isolation e.g., OK = okay what? could we prefix with STATUS_* or HTTP_* ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this.


const CACHE_AVAILABLE = 'caches' in self;

// This function fetches an API response and returns the corresponding json
export default function callApi({
Expand All @@ -26,6 +30,38 @@ export default function callApi({
signal,
};

if (method === 'GET' && CACHE_AVAILABLE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@kristw and I were discussing whether this should be opt-in or not. currently there's no way to opt-out, though I guess if if depends on the server/backend implementing Etags then maybe that's a non-issue.

any thoughts on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could add a new option to callApi, to turn this on/off. In the initial tests I did with dashboards and ETag, some of the dashboards load in half the time, so I think we might want to default this to be enabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have merged and added this new issue for turning conditional request on/off.
#128

return caches.open(CACHE_KEY).then(supersetCache =>
supersetCache
.match(url)
.then(cachedResponse => {
if (cachedResponse) {
// if we have a cached response, send its ETag in the
// `If-None-Match` header in a conditional request
const etag = cachedResponse.headers.get('Etag') as string;
request.headers = { ...request.headers, 'If-None-Match': etag };
}

return fetch(url, request);
})
.then(response => {
if (response.status === NOT_MODIFIED) {
return supersetCache.match(url).then(cachedResponse => {
if (cachedResponse) {
return cachedResponse.clone();
}
throw new Error('Received 304 but no content is cached!');
});
} else if (response.status === OK && response.headers.get('Etag')) {
supersetCache.delete(url);
supersetCache.put(url, response.clone());
}

return response;
}),
);
}

if (
(method === 'POST' || method === 'PATCH' || method === 'PUT') &&
typeof postPayload === 'object'
Expand All @@ -44,5 +80,5 @@ export default function callApi({
request.body = formData;
}

return fetch(url, request); // eslint-disable-line compat/compat
return fetch(url, request);
}
6 changes: 6 additions & 0 deletions packages/superset-ui-connection/src/constants.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// HTTP status codes
export const OK = 200;
export const NOT_MODIFIED = 304;

// Namespace for Cache API
export const CACHE_KEY = '@SUPERSET-UI/CONNECTION';
71 changes: 71 additions & 0 deletions packages/superset-ui-connection/test/callApi/callApi.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/* eslint promise/no-callback-in-promise: 'off' */
import fetchMock from 'fetch-mock';
import callApi from '../../src/callApi/callApi';
import { CACHE_KEY } from '../../src/constants';

import { LOGIN_GLOB } from '../fixtures/constants';
import throwIfCalled from '../utils/throwIfCalled';
Expand All @@ -17,16 +18,23 @@ describe('callApi()', () => {
const mockPostUrl = '/mock/post/url';
const mockPutUrl = '/mock/put/url';
const mockPatchUrl = '/mock/patch/url';
const mockCacheUrl = '/mock/cache/url';

const mockGetPayload = { get: 'payload' };
const mockPostPayload = { post: 'payload' };
const mockPutPayload = { post: 'payload' };
const mockPatchPayload = { post: 'payload' };
const mockCachePayload = {
status: 200,
body: 'BODY',
headers: { Etag: 'etag' },
};

fetchMock.get(mockGetUrl, mockGetPayload);
fetchMock.post(mockPostUrl, mockPostPayload);
fetchMock.put(mockPutUrl, mockPutPayload);
fetchMock.patch(mockPatchUrl, mockPatchPayload);
fetchMock.get(mockCacheUrl, mockCachePayload);

afterEach(fetchMock.reset);

Expand Down Expand Up @@ -292,6 +300,69 @@ describe('callApi()', () => {
});
});

it('caches requests with ETags', () =>
callApi({ url: mockCacheUrl, method: 'GET' }).then(() => {
const calls = fetchMock.calls(mockCacheUrl);
expect(calls).toHaveLength(1);

return caches.open(CACHE_KEY).then(supersetCache =>
supersetCache.match(mockCacheUrl).then(cachedResponse => {
expect(cachedResponse).toBeDefined();

return Promise.resolve();
}),
);
}));

it('sends known ETags in the If-None-Match header', () =>
// first call sets the cache
callApi({ url: mockCacheUrl, method: 'GET' }).then(() => {
const calls = fetchMock.calls(mockCacheUrl);
expect(calls).toHaveLength(1);

// second call sends the Etag in the If-None-Match header
return callApi({ url: mockCacheUrl, method: 'GET' }).then(() => {
const fetchParams = calls[1][1];
const headers = { 'If-None-Match': 'etag' };
expect(calls).toHaveLength(2);
expect(fetchParams.headers).toEqual(expect.objectContaining(headers));

return Promise.resolve();
});
}));

it('reuses cached responses on 304 status', () =>
// first call sets the cache
callApi({ url: mockCacheUrl, method: 'GET' }).then(() => {
const calls = fetchMock.calls(mockCacheUrl);
expect(calls).toHaveLength(1);

// second call reuses the cached payload on a 304
const mockCachedPayload = { status: 304 };
fetchMock.get(mockCacheUrl, mockCachedPayload, { overwriteRoutes: true });

return callApi({ url: mockCacheUrl, method: 'GET' }).then(response => {
expect(calls).toHaveLength(2);
expect(response.body).toEqual('BODY');

return Promise.resolve();
});
}));

it('throws error when cache fails on 304', () => {
// this should never happen, since a 304 is only returned if we have
// the cached response and sent the If-None-Match header
const mockUncachedUrl = '/mock/uncached/url';
const mockCachedPayload = { status: 304 };
fetchMock.get(mockUncachedUrl, mockCachedPayload);

return callApi({ url: mockUncachedUrl, method: 'GET' }).catch(error => {
const calls = fetchMock.calls(mockUncachedUrl);
expect(calls).toHaveLength(1);
expect(error.message).toEqual('Received 304 but no content is cached!');
});
});

it('rejects if the request throws', () => {
const mockErrorUrl = '/mock/error/url';
const mockErrorPayload = { status: 500, statusText: 'Internal error' };
Expand Down
28 changes: 28 additions & 0 deletions test/setupTests.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
const caches = {};

class Cache {
cache: object;
constructor(key: string) {
caches[key] = caches[key] || {};
this.cache = caches[key];
}
match(url: string): Promise<Response | null> {
return new Promise((resolve, reject) => resolve(this.cache[url]));
}
delete(url: string) {
delete this.cache[url];
}
put(url: string, response: Response) {
this.cache[url] = response;
}
};

class Caches {
open(key: string): Promise<Cache> {
return new Promise((resolve, reject) => {
resolve(new Cache(key));
});
}
};

global.caches = new Caches();