Skip to content

Commit 643604b

Browse files
committed
Auto merge of #3141 - Turbo87:chart-js, r=jtgeibel
Replace Google Charts with Chart.js Google Charts is the cause of a significant number of issues on Sentry, and generally it feels bad to load 3rd-party code that could potentially be used for user tracking. This PR replaces our Google Charts usage in the Crate downloads graph with https://www.chartjs.org/. It looks slightly different and does not show the average values yet, but IMHO this is a good first step. Compared to the old implementation it now also has basic error handling and a loading state built-in, and these states are properly unit tested too. <img width="955" alt="Bildschirmfoto 2020-12-30 um 01 00 47" src="https://user-images.githubusercontent.com/141300/103321743-597fb680-4a3b-11eb-8293-6d67916e9d24.png"> r? `@jtgeibel`
2 parents 32c0476 + b1a090a commit 643604b

File tree

12 files changed

+248
-269
lines changed

12 files changed

+248
-269
lines changed

app/components/download-graph.hbs

Lines changed: 30 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,30 @@
1-
{{#if this.loadTask.last.error}}
2-
<span data-test-google-api-error>
3-
There was an error loading the Google Charts code.
4-
Please try again later.
5-
</span>
6-
{{else}}
7-
<div
8-
{{did-insert this.renderChart}}
9-
{{did-update this.renderChart @data}}
10-
...attributes
11-
></div>
12-
{{/if}}
1+
<div
2+
local-class="
3+
wrapper
4+
{{if this.chartjs.loadTask.lastSuccessful.value "auto-height"}}
5+
"
6+
data-test-download-graph
7+
...attributes
8+
{{did-insert this.loadChartJs}}
9+
>
10+
{{#if this.chartjs.loadTask.isRunning}}
11+
<LoadingSpinner local-class="spinner" data-test-spinner />
12+
{{else if this.chartjs.loadTask.lastSuccessful.value}}
13+
<canvas
14+
{{did-insert this.createChart}}
15+
{{did-update this.updateChart @data}}
16+
{{will-destroy this.destroyChart}}
17+
/>
18+
{{else}}
19+
<div local-class="error" data-test-error>
20+
<p>Sorry, there was a problem loading the graphing code.</p>
21+
<button
22+
type="button"
23+
data-test-reload
24+
{{on "click" this.reloadPage}}
25+
>
26+
Try again
27+
</button>
28+
</div>
29+
{{/if}}
30+
</div>

app/components/download-graph.js

Lines changed: 64 additions & 183 deletions
Original file line numberDiff line numberDiff line change
@@ -1,210 +1,91 @@
11
import { action } from '@ember/object';
22
import { inject as service } from '@ember/service';
3+
import { waitForPromise } from '@ember/test-waiters';
34
import Component from '@glimmer/component';
45

5-
import { task } from 'ember-concurrency';
6-
7-
import { ExternalScriptError } from '../services/google-charts';
8-
import { ignoreCancellation } from '../utils/concurrency';
6+
import window from 'ember-window-mock';
97

108
// Colors by http://colorbrewer2.org/#type=diverging&scheme=RdBu&n=10
119
const COLORS = ['#67001f', '#b2182b', '#d6604d', '#f4a582', '#92c5de', '#4393c3', '#2166ac', '#053061'];
10+
const BG_COLORS = ['#d3b5bc', '#eabdc0', '#f3d0ca', '#fce4d9', '#deedf5', '#c9deed', '#2166ac', '#053061'];
1211

1312
export default class DownloadGraph extends Component {
14-
@service googleCharts;
15-
16-
resizeHandler = () => this.renderChart();
17-
18-
constructor() {
19-
super(...arguments);
20-
21-
this.loadTask
22-
.perform()
23-
.catch(ignoreCancellation)
24-
.catch(error => {
25-
// ignore `ExternalScriptError` errors since we handle those in the template
26-
if (!(error instanceof ExternalScriptError)) {
27-
throw error;
28-
}
29-
});
13+
@service chartjs;
3014

31-
window.addEventListener('resize', this.resizeHandler, false);
15+
@action loadChartJs() {
16+
waitForPromise(this.chartjs.loadTask.perform()).catch(() => {
17+
// Ignore Promise rejections. We'll handle them through the derived state properties.
18+
});
3219
}
3320

34-
willDestroy() {
35-
super.willDestroy(...arguments);
36-
window.removeEventListener('resize', this.resizeHandler);
21+
@action createChart(element) {
22+
let Chart = this.chartjs.loadTask.lastSuccessful.value;
23+
24+
this.chart = new Chart(element, {
25+
type: 'line',
26+
data: this.data,
27+
options: {
28+
layout: {
29+
padding: 10,
30+
},
31+
scales: {
32+
xAxes: [{ type: 'time', time: { stepSize: 7, tooltipFormat: 'MMM D', unit: 'day' } }],
33+
yAxes: [{ stacked: true, ticks: { min: 0, precision: 0 } }],
34+
},
35+
tooltips: {
36+
mode: 'index',
37+
intersect: false,
38+
position: 'nearest',
39+
},
40+
},
41+
});
3742
}
3843

39-
@task(function* () {
40-
if (!this.googleCharts.loaded) {
41-
yield this.googleCharts.load();
42-
this.renderChart();
43-
}
44-
})
45-
loadTask;
44+
@action updateChart() {
45+
let { chart, animate } = this.chart;
4646

47-
@action
48-
renderChart(element) {
49-
if (element) {
50-
this.chartElement = element;
51-
} else if (this.chartElement) {
52-
element = this.chartElement;
53-
} else {
54-
return;
55-
}
47+
if (chart) {
48+
chart.data = this.data;
5649

57-
let data = this.args.data;
58-
59-
let subarray_length = (data[1] || []).length;
60-
61-
// Start at 1 to skip the date element in the 0th
62-
// location in the array.
63-
for (let k = 1; k < subarray_length; k++) {
64-
let on = false;
65-
66-
// Start at 1 because the 0th entry in the array
67-
// is an array of version numbers.
68-
//
69-
// End before the last element is reached because we never
70-
// want to change the last element.
71-
for (let i = 1; i < data.length - 1; i++) {
72-
// k + 1 because the first entry in the array is the date
73-
let value = data[i][k];
74-
75-
// If we are "off" and are looking at a zero
76-
// replace the data at this point with `null`.
77-
//
78-
// Null tells google.visualization to stop drawing
79-
// the line altogether.
80-
if (!on && value === 0) {
81-
data[i][k] = null;
82-
}
83-
84-
// If we are off and the value is not zero, we
85-
// need to turn back on. (keep the value the same though)
86-
else if (!on && value !== 0) {
87-
on = true;
88-
89-
// We previously wrote a null into data[i - 1][k + 1],
90-
// so to make the graph look pretty, we'll switch it back
91-
// to the zero that it was before.
92-
if (i >= 2) {
93-
data[i - 1][k] = 0;
94-
}
95-
}
96-
// If we are on and the value is zero, turn off
97-
// but keep the zero in the array
98-
else if (on && value === 0) {
99-
on = false;
100-
}
50+
if (animate) {
51+
chart.update();
52+
} else {
53+
chart.update(0);
10154
}
10255
}
56+
}
10357

104-
let { loaded, visualization } = this.googleCharts;
105-
106-
let show = data && loaded;
107-
element.style.display = show ? '' : 'none';
108-
if (!show) {
109-
return;
110-
}
111-
112-
let myData = visualization.arrayToDataTable(data);
113-
114-
let dateFmt = new visualization.DateFormat({
115-
pattern: 'LLL d, yyyy',
116-
});
117-
dateFmt.format(myData, 0);
118-
119-
// Create a formatter to use for daily download numbers
120-
let numberFormatWhole = new visualization.NumberFormat({
121-
pattern: '#,##0',
122-
});
123-
124-
// Create a formatter to use for 7-day average numbers
125-
let numberFormatDecimal = new visualization.NumberFormat({
126-
pattern: '#,##0.0',
127-
});
58+
@action destroyChart() {
59+
this.chart.destroy();
60+
}
12861

129-
// use a DataView to calculate an x-day moving average
130-
let days = 7;
131-
let view = new visualization.DataView(myData);
132-
let moving_avg_func_for_col = function (col) {
133-
return function (dt, row) {
134-
// For the last rows (the *first* days, remember, the dataset is
135-
// backwards), we cannot calculate the avg. of previous days.
136-
if (row >= dt.getNumberOfRows() - days) {
137-
return null;
138-
}
62+
@action reloadPage() {
63+
window.location.reload();
64+
}
13965

140-
let total = 0;
141-
for (let i = days; i > 0; i--) {
142-
total += dt.getValue(row + i, col);
143-
}
144-
let avg = total / days;
66+
get data() {
67+
let [labels, ...rows] = this.args.data;
68+
69+
let datasets = labels
70+
.slice(1)
71+
.map((label, index) => ({
72+
data: rows.map(row => ({ x: row[0], y: row[index + 1] })),
73+
label: label,
74+
}))
75+
.reverse()
76+
.map(({ label, data }, index) => {
14577
return {
146-
v: avg,
147-
f: numberFormatDecimal.formatValue(avg),
78+
backgroundColor: BG_COLORS[index],
79+
borderColor: COLORS[index],
80+
borderWidth: 2,
81+
cubicInterpolationMode: 'monotone',
82+
data: data,
83+
label: label,
84+
pointHoverBorderWidth: 2,
85+
pointHoverRadius: 5,
14886
};
149-
};
150-
};
151-
152-
let columns = [0]; // 0 = datetime
153-
let seriesOption = {};
154-
let [headers] = data;
155-
// Walk over the headers/colums in reverse order, as the newest version
156-
// is at the end, but in the UI we want it at the top of the chart legend.
157-
158-
range(headers.length - 1, 0, -1).forEach((dataCol, i) => {
159-
// Set the number format for the colum in the data table.
160-
numberFormatWhole.format(myData, dataCol);
161-
columns.push(dataCol); // add the column itself
162-
columns.push({
163-
// add a 'calculated' column, the moving average
164-
type: 'number',
165-
label: `${headers[dataCol]} ${days}-day avg.`,
166-
calc: moving_avg_func_for_col(dataCol),
16787
});
168-
// Note: while the columns start with index 1 (because 0 is the time
169-
// axis), the series configuration starts with index 0.
170-
seriesOption[i * 2] = {
171-
type: 'scatter',
172-
color: COLORS[i % COLORS.length],
173-
pointSize: 3,
174-
pointShape: 'square',
175-
};
176-
seriesOption[i * 2 + 1] = {
177-
type: 'area',
178-
color: COLORS[i % COLORS.length],
179-
lineWidth: 2,
180-
curveType: 'function',
181-
visibleInLegend: false,
182-
};
183-
});
184-
view.setColumns(columns);
185-
186-
let chart = new visualization.ComboChart(element);
187-
chart.draw(view, {
188-
chartArea: { left: 85, width: '77%', height: '80%' },
189-
hAxis: {
190-
minorGridlines: { count: 8 },
191-
},
192-
vAxis: {
193-
minorGridlines: { count: 5 },
194-
viewWindow: { min: 0 },
195-
},
196-
isStacked: true,
197-
focusTarget: 'category',
198-
seriesType: 'scatter',
199-
series: seriesOption,
200-
});
201-
}
202-
}
20388

204-
function range(start, end, step) {
205-
let array = [];
206-
for (let i = start; i !== end; i += step) {
207-
array.push(i);
89+
return { datasets };
20890
}
209-
return array;
21091
}
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
.wrapper {
2+
display: grid;
3+
place-items: center;
4+
border: solid 1px var(--gray-border);
5+
border-radius: 5px;
6+
min-height: 400px;
7+
8+
&.auto-height {
9+
min-height: auto;
10+
}
11+
}
12+
13+
.spinner {
14+
transform: scale(3.0);
15+
}
16+
17+
.error {
18+
text-align: center;
19+
}

app/routes/application.js

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import { shouldPolyfill as shouldPolyfillNumberFormat } from '@formatjs/intl-num
88
import { shouldPolyfill as shouldPolyfillPluralRules } from '@formatjs/intl-pluralrules/should-polyfill';
99

1010
export default class ApplicationRoute extends Route {
11-
@service googleCharts;
1211
@service notifications;
1312
@service progress;
1413
@service session;
@@ -23,12 +22,6 @@ export default class ApplicationRoute extends Route {
2322
// eslint-disable-next-line ember-concurrency/no-perform-without-catch
2423
this.session.loadUserTask.perform();
2524

26-
// start loading the Google Charts JS library already
27-
// and ignore any errors since we will catch them again
28-
// anyway when we call `load()` from the `DownloadGraph`
29-
// component
30-
this.googleCharts.load().catch(() => {});
31-
3225
// load `Intl` polyfills if necessary
3326
let polyfillImports = [];
3427
if (shouldPolyfillGetCanonicalLocales()) {

app/services/chartjs.js

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
import Service from '@ember/service';
2+
3+
import { task } from 'ember-concurrency';
4+
5+
export default class ChartJsLoader extends Service {
6+
@(task(function* () {
7+
let Chart = yield import('chart.js').then(module => module.default);
8+
Chart.platform.disableCSSInjection = true;
9+
return Chart;
10+
}).drop())
11+
loadTask;
12+
}

0 commit comments

Comments
 (0)