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 14:45:27 UTC
[GitHub] [druid] gianm commented on a change in pull request #10271: Web console: fix json input
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