DM-8214 - handling server errors in charts #244
Conversation
| }); | ||
| } | ||
| return errors; | ||
| } |
There was a problem hiding this comment.
-just for your reference, using 'reduce' is nicer than 'forEach' in terms of functional nature.
There was a problem hiding this comment.
Thank you for the note. Indeed, looks like in my code I use forEach a lot, even where simple for loop would be more performant. (See https://jsperf.com/array-reduce-vs-foreach/9) I just find forEach more readable, especially when the values need to be accumulated in an array.
There was a problem hiding this comment.
FYI - we should not worry about performance for stuff like this unless it is clear there is a need. Most of our objects and arrays are so small the speed difference is negligible anyway. Those comparisons will do the same thing like 10,000 times and then compare.
There was a problem hiding this comment.
I agree. The only place where it matters is when processing arrays of hundred thousands elements.
There was a problem hiding this comment.
Object.entries() is a good alternative.
const errors = Object.entries(chartDataElements)
.filter( ([k,v]) -> v.error)
.map( ([k,v]) -> v.error);
|
review completes. good! |
|
Looks great, but Object.entries is a feature of EcmaScript2017, which is in
Draft stage now. Is it OK to use?
I don't remember what decision we made and what ourWebPack setting supports.
2016-11-29 16:52 GMT-08:00 loitly <[email protected]>:
… ***@***.**** commented on this pull request.
------------------------------
In src/firefly/js/charts/ChartsCntlr.js
<#244>:
> + * @param chartId
+ * @returns {Array<{message:string, reason:object}>} an array of error objects
+ */
+export function getErrors(chartId) {
+ const chartDataElements = get(flux.getState(), [CHART_SPACE_PATH, 'data', chartId, 'chartDataElements']);
+ const errors = [];
+ if (chartDataElements) {
+ Object.keys(chartDataElements).forEach((id) => {
+ const error = chartDataElements[id].error;
+ if (error) {
+ errors.push(error);
+ }
+ });
+ }
+ return errors;
+}
Object.entries() is a good alternative.
const errors = Object.entries(chartDataElements)
.filter( ([k,v]) -> v.error)
.map( ([k,v]) -> v.error);
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#244>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/ALU3QEsgfdLjkcPqxT5-I_dR9xV7O7g4ks5rDMjSgaJpZM4K_d7n>
.
|
|
We use babel-polyfill with es2015 and stage-2 presets. Object entries and values are supported in babel-polyfill. I think it should be okay. I've used it in several places. |
This pull request is for handling server error in a chart. Prior to it, loading message would not disappear when an error occurred.
The test relies on a bug in 'Clear' options behavior for histogram. When 'Clear' is pressed, all fields histogram options are cleared, including 'Column or expression value'. If options are applied at this point, an empty column is sent to the server, resulting in an error.