Remove the requirement to pre-format JSON data before it is passed into jsonInput #3937

RyanMG · 2025-03-11T17:32:32Z

Linked issue: #1923

This change attempts to handle the use case of formatting provided JSON values to the jsonInput component, so each use does not need to manually format the value. It does this under narrow criteria here for safety - readonly inputs only being the main requirement, to avoid possible formatting conflicts while a user was editing a value.

Optional discussion

I did want to discuss if we wanted stricter typing on the values that jsonInput would accepts. It currently gets its types from CodeInput, which is turn gets value from HoistInputProps. It is set as value?: any; which not quite right here - I would assume jsonInput would want a more limited set of accepted options? However, I am not yet sure of the full set of XH provided types we would want to limit this to. I see PlainObject, but I am likely missing (or just forgetting) other valid options. So optionally:

export type JsonInputProps = CodeInputProps & {value?: string | PlainObject | null | undefined};

There was at least one case i found in toolbox where a plain object was provided to jsonInput, rather than a JSON string. It sounds like this may be an invalid use case which just happens to work currently? We should not attempt to call the json formatter, as parser + string an object will just output [Object object].

Hoist P/R Checklist

Caught up with develop branch as of last change.
Added CHANGELOG entry, or determined not required.
Reviewed for breaking changes, added breaking-change label + CHANGELOG if so.
Updated doc comments / prop-types, or determined not required.
Reviewed and tested on Mobile, or determined not required.
Created Toolbox branch / PR, or determined not required.

- prevents need for each use of jsonInput to manually format the value

…on pool monitor - will be formatted in the JSONinput component itself

RyanMG · 2025-03-11T17:33:23Z

desktop/cmp/input/JsonInput.ts

@@ -21,11 +21,18 @@ export const [JsonInput, jsonInput] = hoistCmp.withFactory<JsonInputProps>({
    displayName: 'JsonInput',
    className: 'xh-json-input',
    render(props, ref) {
+        let {value, ...jsonInputProps} = props;
+
+        if (typeof value !== 'string') {


~~Originally was checking if value was an object, but null seems to be a valid value to be passed in, and is an object in JS.~~

Per request, now using lodash for this check.

- use lodash for data type checks (rather than typeof) - rename spread props to reference that they are going INTO codeInput, rather than OUT OF jsonInput

haynesjm42 · 2025-03-17T17:24:23Z

desktop/cmp/input/JsonInput.ts

+        let {value, ...codeInputProps} = props;
+
+        if (isPlainObject(value)) {
+            value = JSON.stringify(value, null, 2);


Any processing of the value should be done in the input model, in this case that would be CodeInputModel since this input control is just a wrapper on the codeInput component, might make sense to introduce a JsonInputModel class here for special json handling. HoistInputModel is the base class for all input component models and has methods for converting between the external and internal values, which is where code like this needs to live. It is important to use those methods as our input components support both the controlled mode (providing value and onChange in props) as well as binding to a property of a provided model.

DateInput is probably a good example to follow here, where it has a valueType prop to tell it whether it should be dealing with a LocalDate or a Date.

I'm personally in favor of this change overall, others might feel differently as it does open the door for potentially confusing behavior as the object will be stringified and then parsed when written back, though any usages currently would effectively be doing the same thing, just that they'd just be needing to do the stringifying and parsing themselves.

@amcclain / @lbwexler any thoughts on supporting both strings and objects in JsonInput?

It's a good question - I had not considered it.

My concern was that it could muddy the waters a bit in terms of what value types this component supports. Was thinking we could use a type to represent valid JSON values - typefest has JsonValue which I believe would be it.

However for that to be a valid concern at all, we'd need to actually spec the type of values that inputs support, and as David points out above, HoistInputProps.value is stuck on any. Can we add a generic to support setting the value type on that props interface?

That would be a very different change from pretty-printing - I think that still need to be addressed, see comment on the linked issue, but adding proper value typing seems like a good idea. My only concern is that if we did so we would create work for devs who were unwittingly benefitting from looser typing with any, but that's kinda the idea...

If we did support auto-conversion, believe then the type would be Jsonifiable from typefest.

amcclain · 2025-03-18T13:34:54Z

CHANGELOG.md

+* Updated the `jsonInput` component to auto-format its provided value for display.
+It can accept JSON, stringified JSON or plain objects and output proper pretty-printed JSON in the
+UI - without the need for manual formatting in the call.
+


We had a release underneath this PR, sorry :) - will need to move this up

…n showing readonly values - specific use case here is pretty printing JSON data without manual fromatting of provided values - codeInput does all the heavy lifting, so change is there (possible use cases for other formats) - update to admin activity details use of jsonInput to test change

- no longer attempts to format the JSON for display manually - rename an observed prop to represent that it is not auto-formatted

- includes an update to a component which was passing a raw object into jsonInput - objects will still need to be stringified to use in jsonInput / codeInput - also includes doc update to the codeInput interface

RyanMG · 2025-03-18T18:18:58Z

admin/tabs/activity/tracking/detail/ActivityDetailModel.ts

 import {action, computed, makeObservable, observable} from '@xh/hoist/mobx';
 import {ActivityTrackingModel} from '../ActivityTrackingModel';

 export class ActivityDetailModel extends HoistModel {
    @lookup(ActivityTrackingModel) activityTrackingModel: ActivityTrackingModel;
    @managed gridModel: GridModel;
    @managed formModel: FormModel;
-    @observable formattedData;
+    @observable displayData;


This model is no longer doing the formatting. Update the name to reflect. See https://github.com/xh/hoist-react/pull/3937/files#diff-2a82d78cc59aadd90860fcd0383be76d3b7491d7b19736a18857b6b93d30cd66L117-L125

…SON formatter

RyanMG · 2025-03-18T18:30:23Z

desktop/cmp/input/CodeInput.ts

@@ -326,22 +334,33 @@ class CodeInputModel extends HoistInputModel {
        if (this.cursor) this.clearSearchResults();
    };

-    onAutoFormat = () => {
-        if (!isFunction(this.componentProps.formatter)) return;
+    override toInternal(val: any) {


Override fo the toInternal used by the HoistInputModel. Now will run the formatter if requested and properly configured.

…ter describe the functionality occuring

RyanMG · 2025-03-18T18:34:15Z

admin/tabs/activity/tracking/detail/ActivityDetailView.ts

        width: '100%',
        height: '100%',
        showCopyButton: true,
-        value: model.formattedData
+        value: model.displayData


See https://github.com/xh/hoist-react/pull/3937/files#r2001715715

RyanMG · 2025-03-18T18:37:35Z

desktop/cmp/input/CodeInput.ts

        }
    }

+    private formatAndSetEditorValue() {


@amcclain had some naming / formatting updates he wanted done here.

onAutoFormat -> formatValue was the first pass at the name. We are choosing to use autoFormat as the new prop (on interface) to format readonly data when loading into the UI. Anselm felt onAutoFormat was not the best name of the reaction happening here (to a user requesting the use of the formatter to update their UI)
tryPrettyPrint was use only once, in this method, to run the formatter. Inlining that call to the method itself.
However, we subsequently added safeFormatValue, which is much leaner - it just takes a val, tries to format it, and returns it one way or the other. This existing method was already safe (try/catch block), so to a future developer it may be configuring why the similar names. So I opted to update this method to denote that it would be sending the value to the editor (code mirror) instead of returning said value.

RyanMG · 2025-03-18T18:38:46Z

desktop/cmp/input/JsonInput.ts

-            linter: linter,
-            formatter: fmtJson,
+            linter,
+            formatter: safeFmtJson,


Unsure if we want this? We create a "safe" version of fmtJson (https://github.com/xh/hoist-react/pull/3937/files#diff-1bd2a34b6889e1254a813e22f3fecee2169724007549f9c6d2396658c54ebc7bR76). I made this update to use that for jsonInput, but that may create issues I am not thinking of?

RyanMG · 2025-03-18T18:39:32Z

Had a meeting this morning with @amcclain, and in that we reviewed this issue broadly. Updates done in that meeting applied to this PR. CC:// @haynesjm42

RyanMG · 2025-03-18T19:09:58Z

desktop/cmp/input/CodeInput.ts

@@ -43,6 +43,14 @@ export interface CodeInputProps extends HoistProps, HoistInputProps, LayoutProps
    /** True to focus the control on render. */


This is a desktop component. I did not see a mobile version, but let me know if I just overlooked something.

RyanMG added 2 commits March 11, 2025 09:54

feat(jsonInput): format the provided value to display properly

ef3c359

- prevents need for each use of jsonInput to manually format the value

refactor(jsonInput): remove auto formatting of JSON data for connecti…

4edcfad

…on pool monitor - will be formatted in the JSONinput component itself

RyanMG commented Mar 11, 2025

View reviewed changes

RyanMG requested a review from haynesjm42 March 11, 2025 17:33

RyanMG mentioned this pull request Mar 11, 2025

JsonInput should auto-format the first time it renders data #1923

Open

RyanMG added 2 commits March 11, 2025 15:53

post-pr review updates to jsonInput formatting update

3bcf2ff

- use lodash for data type checks (rather than typeof) - rename spread props to reference that they are going INTO codeInput, rather than OUT OF jsonInput

doc: update CHANGELOG for the jsonInput format update

eae1a14

lbwexler changed the title ~~feat: Remove the requirement to pre-format JSON data before it is passed into jsonInput~~ Remove the requirement to pre-format JSON data before it is passed into jsonInput Mar 12, 2025

haynesjm42 reviewed Mar 17, 2025

View reviewed changes

amcclain reviewed Mar 18, 2025

View reviewed changes

RyanMG added 5 commits March 18, 2025 10:44

clean up activity detail admin JSON display

a9035ef

- no longer attempts to format the JSON for display manually - rename an observed prop to represent that it is not auto-formatted

Merge branch 'develop' into remove-json-preformat

5806d9f

doc: CHANGELOG update to account for version bump

50cbf39

revert original batch of jsonInput changes

27bd0f5

- includes an update to a component which was passing a raw object into jsonInput - objects will still need to be stringified to use in jsonInput / codeInput - also includes doc update to the codeInput interface

RyanMG commented Mar 18, 2025

View reviewed changes

have the default formatter for jsonInput be the safe version of the J…

97b36ec

…SON formatter

RyanMG commented Mar 18, 2025

View reviewed changes

method name update from formatValue -> formatAndSetEditorValue to bet…

ae1a065

…ter describe the functionality occuring

RyanMG commented Mar 18, 2025

View reviewed changes

RyanMG added 2 commits April 8, 2025 19:32

Merge branch 'develop' into remove-json-preformat

d64e207

Remove jest testing code which got stuck and commited on accident

82c3900

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove the requirement to pre-format JSON data before it is passed into jsonInput #3937

Remove the requirement to pre-format JSON data before it is passed into jsonInput #3937

Uh oh!

RyanMG commented Mar 11, 2025 •

edited

Loading

Uh oh!

RyanMG Mar 11, 2025 •

edited

Loading

Uh oh!

haynesjm42 Mar 17, 2025

Uh oh!

amcclain Mar 18, 2025

Uh oh!

amcclain Mar 18, 2025

Uh oh!

amcclain Mar 18, 2025

Uh oh!

RyanMG Mar 18, 2025

Uh oh!

RyanMG Mar 18, 2025

Uh oh!

RyanMG Mar 18, 2025

Uh oh!

RyanMG Mar 18, 2025

Uh oh!

RyanMG Mar 18, 2025

Uh oh!

RyanMG commented Mar 18, 2025

Uh oh!

RyanMG Mar 18, 2025

Uh oh!

Uh oh!

		@@ -43,6 +43,14 @@ export interface CodeInputProps extends HoistProps, HoistInputProps, LayoutProps
		/** True to focus the control on render. */

Remove the requirement to pre-format JSON data before it is passed into jsonInput #3937

Are you sure you want to change the base?

Remove the requirement to pre-format JSON data before it is passed into jsonInput #3937

Uh oh!

Conversation

RyanMG commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Hoist P/R Checklist

Uh oh!

RyanMG Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RyanMG commented Mar 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RyanMG commented Mar 11, 2025 •

edited

Loading

RyanMG Mar 11, 2025 •

edited

Loading