feat: add support for conditional requests #119

betodealmeida · 2019-03-14T10:42:21Z

🏆 Enhancements

This PR adds support for conditional GET requests, since they're not supported by the Fetch API. This is how it works:

If a GET request is successful (status 200 OK) and it has an ETag header, a copy of the response is stored using the Cache API.
The next time a request for the same URL is made, the cache is checked.
If the cache has a cached request, its ETag is retrieved.
The client does a request with the header If-None-Matches with the stored ETag.
If the server returns a status 304 Not Modified, the cached response is used instead.
Otherwise, if the response is a 200 OK the previous cache is deleted and the new response is stored.

This will be used with apache/superset#7032 to allow for conditional requests in Superset.

I started writing unit tests, but the Cache API is not supported neither in Jest (jestjs/jest#7365) nor in jsdom (jsdom/jsdom#2422). I did full end-to-end tests with Superset, verifying that resources are properly cached and retrieved.

codecov · 2019-03-14T10:42:28Z

Codecov Report

Merging #119 into master will decrease coverage by 1.97%.
The diff coverage is 14.28%.

@@            Coverage Diff             @@
##           master     #119      +/-   ##
==========================================
- Coverage     100%   98.02%   -1.98%     
==========================================
  Files          74       73       -1     
  Lines         914      912       -2     
  Branches      220      212       -8     
==========================================
- Hits          914      894      -20     
- Misses          0       17      +17     
- Partials        0        1       +1

Impacted Files	Coverage Δ
...ages/superset-ui-connection/src/callApi/callApi.ts	`51.35% <14.28%> (-48.65%)`	⬇️
...es/superset-ui-dimension/src/computeMaxFontSize.ts	`100% <0%> (ø)`	⬆️
packages/superset-ui-chart/src/query/Metrics.ts	`100% <0%> (ø)`	⬆️
...es/superset-ui-chart/src/query/buildQueryObject.ts	`100% <0%> (ø)`	⬆️
packages/superset-ui-dimension/src/mergeMargin.ts

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57a64b1...d5d2e21. Read the comment docs.

codecov · 2019-03-14T10:42:28Z

Codecov Report

Merging #119 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #119   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          74     77    +3     
  Lines         914    978   +64     
  Branches      220    234   +14     
=====================================
+ Hits          914    978   +64

Impacted Files	Coverage Δ
...ackages/superset-ui-chart/src/models/ChartProps.ts	`100% <ø> (ø)`	⬆️
...kages/superset-ui-chart/src/clients/ChartClient.ts	`100% <100%> (ø)`	⬆️
...ages/superset-ui-connection/src/callApi/callApi.ts	`100% <100%> (ø)`	⬆️
packages/superset-ui-connection/src/constants.ts	`100% <100%> (ø)`
...rset-ui-chart/src/components/ChartDataProvider.tsx	`100% <100%> (ø)`
...kages/superset-ui-connection/src/SupersetClient.ts	`100% <100%> (ø)`	⬆️
...ckages/superset-ui-chart/test/fixtures/formData.ts	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57a64b1...4047db2. Read the comment docs.

betodealmeida · 2019-03-14T11:36:41Z

@williaster, @kristw any thoughts on the unit tests? I know we're aiming for 100% coverage.

williaster

Left a couple comments.

Re testing, this comment in the jest issue you linked to suggests that jsdom does have this now? would that work?

williaster · 2019-03-14T18:18:37Z

packages/superset-ui-connection/src/callApi/callApi.ts

@@ -1,6 +1,10 @@
 import 'whatwg-fetch';
 import { CallApi } from '../types';

+const cacheAvailable = 'caches' in self;
+const NOT_MODIFIED = 304;


response numbers should be in a separate constants file

Thanks, will do!

williaster · 2019-03-14T18:19:09Z

packages/superset-ui-connection/src/callApi/callApi.ts

@@ -1,6 +1,10 @@
 import 'whatwg-fetch';
 import { CallApi } from '../types';

+const cacheAvailable = 'caches' in self;


I'd UPPER_CASE this since it's used as a constant in this file

williaster · 2019-03-14T18:20:58Z

packages/superset-ui-connection/src/callApi/callApi.ts

@@ -26,6 +30,41 @@ export default function callApi({
    signal,
  };

+  if (method === 'GET' && cacheAvailable) {
+    return caches.open('superset').then(supersetCache =>


the superset key should also be a constant in a separate file, and maybe more unique, matching the package name seems safer (even though it sounds like Caches are scoped to by origin) e.g., @SUPERSET-UI/CONNECTION?

Good point.

williaster · 2019-03-14T18:22:57Z

packages/superset-ui-connection/src/callApi/callApi.ts

+            }
+          }
+
+          return fetch(url, request); // eslint-disable-line compat/compat


since we have multiple occurrences of this now we should just move this to the top of the file /* eslint compat/compat: 'off' */

williaster · 2019-03-14T18:24:31Z

packages/superset-ui-connection/src/callApi/callApi.ts

+            // `If-None-Match` header in a conditional request
+            const etag = cachedResponse.headers.get('Etag');
+            if (etag) {
+              request.headers = request.headers || {};


could simplify to request.headers = { ...request.headers, ['If-None-Match']: etag }

Cool, I didn't know the spread operator worked with nulls. Will do.

williaster · 2019-03-14T18:42:40Z

packages/superset-ui-connection/README.md

@@ -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


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

"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.

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.

betodealmeida · 2019-03-14T20:47:17Z

@williaster I'll try to use one of those new packages to write the tests.

betodealmeida · 2019-03-18T21:18:20Z

@williaster I added unit tests and got the coverage up to 100%.

kristw · 2019-03-19T00:54:56Z

Could you add another test case when CACHE_AVAILABLE is false?

williaster

thanks for adding tests and getting them to one hundo 💯

@kristw and I had a couple more small comments but looks good overall.

williaster · 2019-03-19T04:19:47Z

package.json

+        "caches": true
+      },
+      "setupFiles": [
+        "<rootDir>/test/setupTests.ts"


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

williaster · 2019-03-19T04:22:44Z

packages/superset-ui-connection/src/callApi/callApi.ts

 import 'whatwg-fetch';
 import { CallApi } from '../types';
+import { CACHE_KEY, NOT_MODIFIED, OK } from '../constants';


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_* ?

Fixed this.

williaster · 2019-03-19T04:24:57Z

packages/superset-ui-connection/src/callApi/callApi.ts

@@ -26,6 +30,38 @@ export default function callApi({
    signal,
  };

+  if (method === 'GET' && CACHE_AVAILABLE) {


@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?

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.

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

betodealmeida · 2019-04-02T17:18:30Z

Ping @kristw @williaster

Smilebags · 2020-04-08T04:30:11Z

Hey, I'm coming here to debug an issue I'm seeing where changes to the superset code aren't reflected in changed behaviour/UI when accessing superset, and wondered whether that might be because some sort of cache is enabled during development. Deleting entire folders of JS assets (the dist folder) has no impact on the functionality of superset (even when the HTTP cache is disabled) which leads me to believe there's a backend cache being used. Any guidance/direction appreciated.

feat: add support for conditional requests

d5d2e21

betodealmeida requested a review from a team as a code owner March 14, 2019 10:42

betodealmeida added #enhancement New feature or request reviewable Ready for review labels Mar 14, 2019

betodealmeida mentioned this pull request Mar 14, 2019

Fetch charts with GET to benefit from browser cache and conditional requests apache/superset#7032

Merged

williaster reviewed Mar 14, 2019

View reviewed changes

betodealmeida added 2 commits March 17, 2019 01:56

feat: add unit tests for conditional requests

51c0f04

feat: use invariant

0a18b51

betodealmeida removed the reviewable Ready for review label Mar 18, 2019

betodealmeida added 2 commits March 18, 2019 13:58

feat: add type guard

33acdac

feat: fix lint

c5f11da

betodealmeida added the reviewable Ready for review label Mar 18, 2019

williaster reviewed Mar 19, 2019

View reviewed changes

feat: add more unit tests

4047db2

kristw approved these changes Apr 2, 2019

View reviewed changes

kristw merged commit 2ca55ed into master Apr 2, 2019

delete-merged-branch bot deleted the VIZ-334 branch April 2, 2019 17:45

kristw mentioned this pull request Apr 2, 2019

add a new option to callApi, to turn conditional request on/off. #128

Closed

betodealmeida mentioned this pull request Apr 21, 2019

Caching charts in Superset apache/superset#7340

Closed

kristw added a commit that referenced this pull request Apr 17, 2020

fix: move series in front of xy gridlines (#119)

32850ef

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for conditional requests #119

feat: add support for conditional requests #119

betodealmeida commented Mar 14, 2019

codecov bot commented Mar 14, 2019

codecov bot commented Mar 14, 2019 •

edited

Loading

betodealmeida commented Mar 14, 2019

williaster left a comment

williaster Mar 14, 2019

betodealmeida Mar 14, 2019

williaster Mar 14, 2019

betodealmeida Mar 14, 2019

williaster Mar 14, 2019

betodealmeida Mar 14, 2019

williaster Mar 14, 2019

betodealmeida Mar 14, 2019

williaster Mar 14, 2019

betodealmeida Mar 14, 2019

williaster Mar 14, 2019

betodealmeida Mar 14, 2019

betodealmeida Mar 14, 2019

betodealmeida commented Mar 14, 2019

betodealmeida commented Mar 18, 2019

kristw commented Mar 19, 2019

williaster left a comment

williaster Mar 19, 2019

williaster Mar 19, 2019

betodealmeida Mar 21, 2019

williaster Mar 19, 2019

betodealmeida Mar 21, 2019

kristw Apr 2, 2019

betodealmeida commented Apr 2, 2019

Smilebags commented Apr 8, 2020

feat: add support for conditional requests #119

feat: add support for conditional requests #119

Conversation

betodealmeida commented Mar 14, 2019

codecov bot commented Mar 14, 2019

Codecov Report

codecov bot commented Mar 14, 2019 • edited Loading

Codecov Report

betodealmeida commented Mar 14, 2019

williaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

betodealmeida commented Mar 14, 2019

betodealmeida commented Mar 18, 2019

kristw commented Mar 19, 2019

williaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

betodealmeida commented Apr 2, 2019

Smilebags commented Apr 8, 2020

codecov bot commented Mar 14, 2019 •

edited

Loading