-
Notifications
You must be signed in to change notification settings - Fork 16
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 warning to widgets when column is missing #427
Changes from 8 commits
70f9d28
d131060
f5b6c9b
57db12d
65d5ba4
ae8b092
e2e44c5
d9868f5
d9f50bc
e6b6ae8
1562ce2
86471ec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
const NAME = 'InvalidColumnError'; | ||
const MESSAGE_START = `Uncaught ${NAME}:`; | ||
|
||
export class InvalidColumnError extends Error { | ||
static message = | ||
'The column selected for this widget is no longer available in the data source.'; | ||
|
||
constructor(message) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My proposition would be to change this to
and in is just test for existence of |
||
super(message); | ||
this.name = NAME; | ||
} | ||
|
||
static is(error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know. This whole mechanism of detecting type of exception by details of how react-workers pass errors seems fishy. First of all, this is not "general" check for It would be really better to have error codes, and proper error propagation between workers and main thread. Anyway, i. am worried that all errors throws by "remote" api calls don't have "Uncaught: InvalidColumnError" prefix and thus don't pass this test ... and as result don't trigger warning. |
||
return error.message?.startsWith(MESSAGE_START); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
import { InvalidColumnError } from '@carto/react-core/'; | ||
import { act, render, screen } from '@testing-library/react'; | ||
import React from 'react'; | ||
import useWidgetFetch from '../../src/hooks/useWidgetFetch'; | ||
|
@@ -79,17 +80,45 @@ describe('useWidgetFetch', () => { | |
await act(() => sleep(250)); | ||
expect(screen.queryByText('loading')).not.toBeInTheDocument(); | ||
|
||
expect(onError).toBeCalled(); | ||
expect(onError).toBeCalledTimes(1); | ||
|
||
modelFn.mockRejectedValue( | ||
new InvalidColumnError('Uncaught InvalidColumnError: Invalid column') | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As per comment above, please add test for "new InvalidColumnError("whatever") - those errors will be thrown by "fromRemote" calls as they will go through dealWithApiError error handling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've just changed this error to ' new InvalidColumnError('FooBar')' and it fails to show warning. |
||
|
||
rerender( | ||
<TestComponent | ||
modelFn={modelFn} | ||
args={{ | ||
id: 'test', | ||
dataSource: 'test', | ||
params: PARAMS_MOCK, | ||
global: false, | ||
onError | ||
}} | ||
/> | ||
); | ||
|
||
expect(screen.getByText('loading')).toBeInTheDocument(); | ||
await act(() => sleep(250)); | ||
expect(screen.queryByText('loading')).not.toBeInTheDocument(); | ||
expect(onError).toBeCalledTimes(1); | ||
expect(screen.queryByText(InvalidColumnError.message)).toBeInTheDocument(); | ||
}); | ||
}); | ||
|
||
// Aux | ||
function TestComponent({ modelFn, args }) { | ||
const { data, isLoading } = useWidgetFetch(modelFn, args); | ||
const { data, isLoading, warning } = useWidgetFetch(modelFn, args); | ||
|
||
if (isLoading) { | ||
return <div>loading</div>; | ||
} | ||
|
||
if (warning) { | ||
return <div>{warning}</div>; | ||
} | ||
|
||
return <div>{data}</div>; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: i don't think message copy belongs to "react-core" and exception type, it's purely widgets & UI matter (and it's used only once there) so maybe move it there?
This should be in
[useWidgetFetch.js]
(or in UI component).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It used in @carto/react-api also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, now I read it correctly. I can move the message to the useWidgetFetch, ok.