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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
