You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/08/13 01:42:06 UTC
[GitHub] [druid] vogievetsky opened a new pull request #10271: Web console: fix json input
vogievetsky opened a new pull request #10271:
URL: https://github.com/apache/druid/pull/10271
Fixed and improved the JSON Input component (as seen in the Edit Spec step of the data loader)
Specifically fixed the issue with the JSON input sometimes moving the cursor when editing, fixed the undo stack.
The fix is based around maintaining the state of the JSON Input in a single state variable (`InternalValue`) and thus not letting it get desynchronized.
While at it I also added a nice error display in case there is an issue with the JSON
Clicking on it takes your cursor to the place with the error which is sweet - doing this required me to nest the Ace editor inside a div which causes a lot of snapshot diffs.
Also updated the snapshots for all the users of JSON Input to shallow render so in the future they would be immune to JSON Input structure changes.
This PR has:
- [x] been self-reviewed.
- [x] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links
- [x] added unit tests or modified existing tests to cover new code paths
- [x] been tested in a test Druid cluster.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #10271: Web console: fix json input
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10271:
URL: https://github.com/apache/druid/pull/10271#discussion_r469941992
##########
File path: web-console/src/components/json-input/json-input.tsx
##########
@@ -37,6 +38,17 @@ function stringifyJson(item: any): string {
}
}
+// Not the best way to check for deep equality but good enough for what we need
Review comment:
What's not the best about it and why is it good enough?
##########
File path: web-console/src/components/auto-form/__snapshots__/auto-form.spec.tsx.snap
##########
@@ -2,521 +2,135 @@
exports[`auto-form snapshot matches snapshot 1`] = `
<div
- class="auto-form"
Review comment:
My understanding is that these `__snapshot__` files are automatically generated for testing purposes. @vogievetsky how would you suggest reviewing them?
##########
File path: web-console/src/components/json-input/json-input.tsx
##########
@@ -48,52 +60,93 @@ interface JsonInputProps {
export const JsonInput = React.memo(function JsonInput(props: JsonInputProps) {
const { onChange, placeholder, focus, width, height, value } = props;
- const stringifiedValue = stringifyJson(value);
- const [stringValue, setStringValue] = useState(stringifiedValue);
- const [blurred, setBlurred] = useState(false);
-
- let parsedValue: any;
- try {
- parsedValue = parseHjson(stringValue);
- } catch {}
- if (typeof parsedValue !== 'object') parsedValue = undefined;
+ const [internalValue, setInternalValue] = useState<InternalValue>(() => ({
+ value,
+ stringified: stringifyJson(value),
+ }));
+ const [showErrorIfNeeded, setShowErrorIfNeeded] = useState(false);
+ const aceEditor = useRef<Editor | undefined>();
- if (parsedValue !== undefined && stringifyJson(parsedValue) !== stringifiedValue) {
- setStringValue(stringifiedValue);
- }
+ useEffect(() => {
+ if (!deepEqual(value, internalValue.value)) {
+ setInternalValue({
+ value,
+ stringified: stringifyJson(value),
+ });
+ }
+ }, [value]);
+ const internalValueError = internalValue.error;
return (
- <AceEditor
- className={classNames('json-input', { invalid: parsedValue === undefined && blurred })}
- mode="hjson"
- theme="solarized_dark"
- onChange={(inputJson: string) => {
- try {
- const value = parseHjson(inputJson);
- onChange(value);
- } catch {}
- setStringValue(inputJson);
- }}
- onFocus={() => setBlurred(false)}
- onBlur={() => setBlurred(true)}
- focus={focus}
- fontSize={12}
- width={width || '100%'}
- height={height || '8vh'}
- showPrintMargin={false}
- showGutter={false}
- value={stringValue}
- placeholder={placeholder}
- editorProps={{
- $blockScrolling: Infinity,
- }}
- setOptions={{
- enableBasicAutocompletion: false,
- enableLiveAutocompletion: false,
- showLineNumbers: false,
- tabSize: 2,
- }}
- style={{}}
- />
+ <div className={classNames('json-input', { invalid: showErrorIfNeeded && internalValueError })}>
+ <AceEditor
+ mode="hjson"
+ theme="solarized_dark"
+ onChange={(inputJson: string) => {
+ let value: any;
+ let error: Error | undefined;
+ try {
+ value = parseHjson(inputJson);
+ } catch (e) {
+ error = e;
+ }
+
+ setInternalValue({
+ value,
+ error,
+ stringified: inputJson,
+ });
+
+ if (!error) {
+ onChange(value);
+ }
+
+ if (showErrorIfNeeded) {
+ setShowErrorIfNeeded(false);
+ }
+ }}
+ onBlur={() => setShowErrorIfNeeded(true)}
+ focus={focus}
+ fontSize={12}
+ width={width || '100%'}
+ height={height || '8vh'}
+ showPrintMargin={false}
+ showGutter={false}
+ value={internalValue.stringified}
+ placeholder={placeholder}
+ editorProps={{
+ $blockScrolling: Infinity,
+ }}
+ setOptions={{
+ enableBasicAutocompletion: false,
+ enableLiveAutocompletion: false,
+ showLineNumbers: false,
+ tabSize: 2,
+ }}
+ style={{}}
+ onLoad={(editor: any) => {
+ aceEditor.current = editor;
+ }}
+ />
+ {showErrorIfNeeded && internalValueError && (
+ <div
+ className="json-error"
+ onClick={() => {
+ if (!aceEditor.current || !internalValueError) return;
+
+ // Message would be something like:
+ // `Found '}' where a key name was expected at line 26,7`
Review comment:
😱
##########
File path: web-console/src/components/json-input/json-input.tsx
##########
@@ -48,52 +60,93 @@ interface JsonInputProps {
export const JsonInput = React.memo(function JsonInput(props: JsonInputProps) {
const { onChange, placeholder, focus, width, height, value } = props;
- const stringifiedValue = stringifyJson(value);
- const [stringValue, setStringValue] = useState(stringifiedValue);
- const [blurred, setBlurred] = useState(false);
-
- let parsedValue: any;
- try {
- parsedValue = parseHjson(stringValue);
- } catch {}
- if (typeof parsedValue !== 'object') parsedValue = undefined;
+ const [internalValue, setInternalValue] = useState<InternalValue>(() => ({
+ value,
+ stringified: stringifyJson(value),
+ }));
+ const [showErrorIfNeeded, setShowErrorIfNeeded] = useState(false);
+ const aceEditor = useRef<Editor | undefined>();
- if (parsedValue !== undefined && stringifyJson(parsedValue) !== stringifiedValue) {
- setStringValue(stringifiedValue);
- }
+ useEffect(() => {
+ if (!deepEqual(value, internalValue.value)) {
+ setInternalValue({
+ value,
+ stringified: stringifyJson(value),
+ });
+ }
+ }, [value]);
+ const internalValueError = internalValue.error;
return (
- <AceEditor
- className={classNames('json-input', { invalid: parsedValue === undefined && blurred })}
- mode="hjson"
- theme="solarized_dark"
- onChange={(inputJson: string) => {
- try {
- const value = parseHjson(inputJson);
- onChange(value);
- } catch {}
- setStringValue(inputJson);
- }}
- onFocus={() => setBlurred(false)}
- onBlur={() => setBlurred(true)}
- focus={focus}
- fontSize={12}
- width={width || '100%'}
- height={height || '8vh'}
- showPrintMargin={false}
- showGutter={false}
- value={stringValue}
- placeholder={placeholder}
- editorProps={{
- $blockScrolling: Infinity,
- }}
- setOptions={{
- enableBasicAutocompletion: false,
- enableLiveAutocompletion: false,
- showLineNumbers: false,
- tabSize: 2,
- }}
- style={{}}
- />
+ <div className={classNames('json-input', { invalid: showErrorIfNeeded && internalValueError })}>
+ <AceEditor
+ mode="hjson"
+ theme="solarized_dark"
+ onChange={(inputJson: string) => {
+ let value: any;
+ let error: Error | undefined;
+ try {
+ value = parseHjson(inputJson);
+ } catch (e) {
+ error = e;
+ }
+
+ setInternalValue({
+ value,
+ error,
+ stringified: inputJson,
+ });
+
+ if (!error) {
+ onChange(value);
+ }
+
+ if (showErrorIfNeeded) {
+ setShowErrorIfNeeded(false);
+ }
+ }}
+ onBlur={() => setShowErrorIfNeeded(true)}
+ focus={focus}
+ fontSize={12}
+ width={width || '100%'}
+ height={height || '8vh'}
+ showPrintMargin={false}
+ showGutter={false}
+ value={internalValue.stringified}
+ placeholder={placeholder}
+ editorProps={{
+ $blockScrolling: Infinity,
+ }}
+ setOptions={{
+ enableBasicAutocompletion: false,
+ enableLiveAutocompletion: false,
+ showLineNumbers: false,
+ tabSize: 2,
+ }}
+ style={{}}
+ onLoad={(editor: any) => {
+ aceEditor.current = editor;
+ }}
+ />
+ {showErrorIfNeeded && internalValueError && (
+ <div
+ className="json-error"
+ onClick={() => {
+ if (!aceEditor.current || !internalValueError) return;
+
+ // Message would be something like:
+ // `Found '}' where a key name was expected at line 26,7`
Review comment:
I suppose this won't always work; is that ok?
##########
File path: web-console/src/components/auto-form/auto-form.spec.tsx
##########
@@ -16,14 +16,14 @@
* limitations under the License.
*/
-import { render } from '@testing-library/react';
+import { shallow } from 'enzyme';
Review comment:
Is this also test code?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vogievetsky commented on a change in pull request #10271: Web console: fix json input
Posted by GitBox <gi...@apache.org>.
vogievetsky commented on a change in pull request #10271:
URL: https://github.com/apache/druid/pull/10271#discussion_r470077857
##########
File path: web-console/src/components/auto-form/auto-form.spec.tsx
##########
@@ -16,14 +16,14 @@
* limitations under the License.
*/
-import { render } from '@testing-library/react';
+import { shallow } from 'enzyme';
Review comment:
Yup, this changes the virtual rendered from a DOM renderer `import { render } from '@testing-library/react';` to a shallow react component level renderer `import { shallow } from 'enzyme';` for the snapshots.
This change makes sense because the auto-form does not really make any of its own cool DOM it only assembles other components so testing their deep structure should not be its concern - otherwise it will have structural changes in the snapshots every-time someone changes the JSON Input
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vogievetsky commented on a change in pull request #10271: Web console: fix json input
Posted by GitBox <gi...@apache.org>.
vogievetsky commented on a change in pull request #10271:
URL: https://github.com/apache/druid/pull/10271#discussion_r470076284
##########
File path: web-console/src/components/auto-form/__snapshots__/auto-form.spec.tsx.snap
##########
@@ -2,521 +2,135 @@
exports[`auto-form snapshot matches snapshot 1`] = `
<div
- class="auto-form"
Review comment:
That is correct.
Here is how I review them:
```
The snapshot change is either tiny or structural (huge) there is no in between.
If the change is tiny:
Review the changed lines and understand what caused that change. Was a class or some other property added / removed? Does it make sense?
If the change is structural:
Do not review the individual lines, only review which snapshot files changed. Does it make sense that the snapshot of this component changed so drastically?
```
In this case all the changes are structural. I have wrapped the JSON Input in an extra `div` (for good reason) which causes all the snapshots of its inner workings to be indented by an extra level. This causes huge diffs. Seeing as I will have huge changed in these snapshots anyway I changed them to be shallow rendered (which they should have been from the start - but better late than never). This explains the structural nature of the change. You are free to comment on whether or not you think it is a good idea. (see below)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vogievetsky commented on a change in pull request #10271: Web console: fix json input
Posted by GitBox <gi...@apache.org>.
vogievetsky commented on a change in pull request #10271:
URL: https://github.com/apache/druid/pull/10271#discussion_r470080584
##########
File path: web-console/src/components/json-input/json-input.tsx
##########
@@ -48,52 +60,93 @@ interface JsonInputProps {
export const JsonInput = React.memo(function JsonInput(props: JsonInputProps) {
const { onChange, placeholder, focus, width, height, value } = props;
- const stringifiedValue = stringifyJson(value);
- const [stringValue, setStringValue] = useState(stringifiedValue);
- const [blurred, setBlurred] = useState(false);
-
- let parsedValue: any;
- try {
- parsedValue = parseHjson(stringValue);
- } catch {}
- if (typeof parsedValue !== 'object') parsedValue = undefined;
+ const [internalValue, setInternalValue] = useState<InternalValue>(() => ({
+ value,
+ stringified: stringifyJson(value),
+ }));
+ const [showErrorIfNeeded, setShowErrorIfNeeded] = useState(false);
+ const aceEditor = useRef<Editor | undefined>();
- if (parsedValue !== undefined && stringifyJson(parsedValue) !== stringifiedValue) {
- setStringValue(stringifiedValue);
- }
+ useEffect(() => {
+ if (!deepEqual(value, internalValue.value)) {
+ setInternalValue({
+ value,
+ stringified: stringifyJson(value),
+ });
+ }
+ }, [value]);
+ const internalValueError = internalValue.error;
return (
- <AceEditor
- className={classNames('json-input', { invalid: parsedValue === undefined && blurred })}
- mode="hjson"
- theme="solarized_dark"
- onChange={(inputJson: string) => {
- try {
- const value = parseHjson(inputJson);
- onChange(value);
- } catch {}
- setStringValue(inputJson);
- }}
- onFocus={() => setBlurred(false)}
- onBlur={() => setBlurred(true)}
- focus={focus}
- fontSize={12}
- width={width || '100%'}
- height={height || '8vh'}
- showPrintMargin={false}
- showGutter={false}
- value={stringValue}
- placeholder={placeholder}
- editorProps={{
- $blockScrolling: Infinity,
- }}
- setOptions={{
- enableBasicAutocompletion: false,
- enableLiveAutocompletion: false,
- showLineNumbers: false,
- tabSize: 2,
- }}
- style={{}}
- />
+ <div className={classNames('json-input', { invalid: showErrorIfNeeded && internalValueError })}>
+ <AceEditor
+ mode="hjson"
+ theme="solarized_dark"
+ onChange={(inputJson: string) => {
+ let value: any;
+ let error: Error | undefined;
+ try {
+ value = parseHjson(inputJson);
+ } catch (e) {
+ error = e;
+ }
+
+ setInternalValue({
+ value,
+ error,
+ stringified: inputJson,
+ });
+
+ if (!error) {
+ onChange(value);
+ }
+
+ if (showErrorIfNeeded) {
+ setShowErrorIfNeeded(false);
+ }
+ }}
+ onBlur={() => setShowErrorIfNeeded(true)}
+ focus={focus}
+ fontSize={12}
+ width={width || '100%'}
+ height={height || '8vh'}
+ showPrintMargin={false}
+ showGutter={false}
+ value={internalValue.stringified}
+ placeholder={placeholder}
+ editorProps={{
+ $blockScrolling: Infinity,
+ }}
+ setOptions={{
+ enableBasicAutocompletion: false,
+ enableLiveAutocompletion: false,
+ showLineNumbers: false,
+ tabSize: 2,
+ }}
+ style={{}}
+ onLoad={(editor: any) => {
+ aceEditor.current = editor;
+ }}
+ />
+ {showErrorIfNeeded && internalValueError && (
+ <div
+ className="json-error"
+ onClick={() => {
+ if (!aceEditor.current || !internalValueError) return;
+
+ // Message would be something like:
+ // `Found '}' where a key name was expected at line 26,7`
Review comment:
Yeah, it it does not find `/line (\d+),(\d+)/` it would just not do anything due to the `if (!m) return;` which is cool I think. But as I write this I realize it would be great to add a test to make sure that if the error format changes in the Hjson library in a future version we catch that - will do.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vogievetsky commented on a change in pull request #10271: Web console: fix json input
Posted by GitBox <gi...@apache.org>.
vogievetsky commented on a change in pull request #10271:
URL: https://github.com/apache/druid/pull/10271#discussion_r470071450
##########
File path: web-console/src/components/json-input/json-input.tsx
##########
@@ -37,6 +38,17 @@ function stringifyJson(item: any): string {
}
}
+// Not the best way to check for deep equality but good enough for what we need
Review comment:
Here is an in depth look at it https://www.mattzeunert.com/2016/01/28/javascript-deep-equal.html
TL;DR the method implemented here would not work for this case for example:
`JSON.stringify({a: 1, b: 2}) !== JSON.stringify({b: 2, a: 1})`
But that is ok because in practice the object output from the JSON input will be the same it later gets back. And if that check fails the worst that will happen is that the cursor would jump. It is also hard to find a decent object deep-equals library that does not come with a bunch of dependencies (as it tries to do a 100% thorough job) and a lot of file size.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vogievetsky commented on a change in pull request #10271: Web console: fix json input
Posted by GitBox <gi...@apache.org>.
vogievetsky commented on a change in pull request #10271:
URL: https://github.com/apache/druid/pull/10271#discussion_r470088258
##########
File path: web-console/src/components/json-input/json-input.tsx
##########
@@ -48,52 +60,93 @@ interface JsonInputProps {
export const JsonInput = React.memo(function JsonInput(props: JsonInputProps) {
const { onChange, placeholder, focus, width, height, value } = props;
- const stringifiedValue = stringifyJson(value);
- const [stringValue, setStringValue] = useState(stringifiedValue);
- const [blurred, setBlurred] = useState(false);
-
- let parsedValue: any;
- try {
- parsedValue = parseHjson(stringValue);
- } catch {}
- if (typeof parsedValue !== 'object') parsedValue = undefined;
+ const [internalValue, setInternalValue] = useState<InternalValue>(() => ({
+ value,
+ stringified: stringifyJson(value),
+ }));
+ const [showErrorIfNeeded, setShowErrorIfNeeded] = useState(false);
+ const aceEditor = useRef<Editor | undefined>();
- if (parsedValue !== undefined && stringifyJson(parsedValue) !== stringifiedValue) {
- setStringValue(stringifiedValue);
- }
+ useEffect(() => {
+ if (!deepEqual(value, internalValue.value)) {
+ setInternalValue({
+ value,
+ stringified: stringifyJson(value),
+ });
+ }
+ }, [value]);
+ const internalValueError = internalValue.error;
return (
- <AceEditor
- className={classNames('json-input', { invalid: parsedValue === undefined && blurred })}
- mode="hjson"
- theme="solarized_dark"
- onChange={(inputJson: string) => {
- try {
- const value = parseHjson(inputJson);
- onChange(value);
- } catch {}
- setStringValue(inputJson);
- }}
- onFocus={() => setBlurred(false)}
- onBlur={() => setBlurred(true)}
- focus={focus}
- fontSize={12}
- width={width || '100%'}
- height={height || '8vh'}
- showPrintMargin={false}
- showGutter={false}
- value={stringValue}
- placeholder={placeholder}
- editorProps={{
- $blockScrolling: Infinity,
- }}
- setOptions={{
- enableBasicAutocompletion: false,
- enableLiveAutocompletion: false,
- showLineNumbers: false,
- tabSize: 2,
- }}
- style={{}}
- />
+ <div className={classNames('json-input', { invalid: showErrorIfNeeded && internalValueError })}>
+ <AceEditor
+ mode="hjson"
+ theme="solarized_dark"
+ onChange={(inputJson: string) => {
+ let value: any;
+ let error: Error | undefined;
+ try {
+ value = parseHjson(inputJson);
+ } catch (e) {
+ error = e;
+ }
+
+ setInternalValue({
+ value,
+ error,
+ stringified: inputJson,
+ });
+
+ if (!error) {
+ onChange(value);
+ }
+
+ if (showErrorIfNeeded) {
+ setShowErrorIfNeeded(false);
+ }
+ }}
+ onBlur={() => setShowErrorIfNeeded(true)}
+ focus={focus}
+ fontSize={12}
+ width={width || '100%'}
+ height={height || '8vh'}
+ showPrintMargin={false}
+ showGutter={false}
+ value={internalValue.stringified}
+ placeholder={placeholder}
+ editorProps={{
+ $blockScrolling: Infinity,
+ }}
+ setOptions={{
+ enableBasicAutocompletion: false,
+ enableLiveAutocompletion: false,
+ showLineNumbers: false,
+ tabSize: 2,
+ }}
+ style={{}}
+ onLoad={(editor: any) => {
+ aceEditor.current = editor;
+ }}
+ />
+ {showErrorIfNeeded && internalValueError && (
+ <div
+ className="json-error"
+ onClick={() => {
+ if (!aceEditor.current || !internalValueError) return;
+
+ // Message would be something like:
+ // `Found '}' where a key name was expected at line 26,7`
Review comment:
Done https://github.com/apache/druid/pull/10271/commits/8eb77e92e23487a0c391e770bcec010545758c6e
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm merged pull request #10271: Web console: fix json input
Posted by GitBox <gi...@apache.org>.
gianm merged pull request #10271:
URL: https://github.com/apache/druid/pull/10271
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org