Add null checks for ArrayField (New PR for #1553)

[richmahn]

This is a new PR to replace PR #1553 since that PR's author is no longer responding. Resolves conflicts.

[ri0ter]

Hi @richmahn is that PR still valid?
Does it require some more work (besides update/rebase), something I could help with?

@epicfaace do you think that this PR could be merged?

[jsani-radar]

wanted to bump this PR, commented on the original issue: #1346 (comment) - let me know if I can assist in getting this through at all

[epicfaace]

Thanks for updating the PR! I'm a bit confused about what exactly the logic of the additions is -- when exactly would keyedFormData be equal to null? Wouldn't it always be equal to [] if the form data itself is equal to null? (see

react-jsonschema-form/packages/core/src/components/fields/ArrayField.js

Lines 184 to 193 in a4f6c1f

	function generateKeyedFormData(formData) {
	return !Array.isArray(formData)
	? []
	: formData.map(item => {
	return {
	key: generateRowId(),
	item,
	};
	});
	}

)?

[richmahn]

@epicfaace The keyDataValue being null happens whenever the json file has a null, such as:

{ "tags": null }

Before it was only allowing it to be { "tags": [] }

I have no idea why it is failing to build/deploy. Says that cross-env doesn't exist on netlify.

[ri0ter]

@epicfaace I did investigate it further and as you are right that generateKeyedFormData provides default to [] but it is being called only in constructor to set the state. Now in getDerivedStateFromProps it receives the same props (in this case null) which has no fallback to [] which leads to the error.

Another thing (could be separate issue) is if you set the type to nullable it won't work, it will crash the same way. But in this case I would assume that nullable check should happen eariler and it should not even enter ArrayField component.

I wanted to mention that there are other issues opened that seems to be related to this one: #1957, #282, #2153

[epicfaace]

I did investigate it further and as you are right that generateKeyedFormData provides default to [] but it is being called only in constructor to set the state. Now in getDerivedStateFromProps it receives the same props (in this case null) which has no fallback to [] which leads to the error.

Ah, I see. So do you think a better solution might be to instead modify getDerivedStateFromProps in some way so that it gives us a default of [] for keyedFormData? That way, we won't need all these null checks.

react-jsonschema-form/packages/core/src/components/fields/ArrayField.js

Lines 222 to 245 in 49e9184

	static getDerivedStateFromProps(nextProps, prevState) {
	// Don't call getDerivedStateFromProps if keyed formdata was just updated.
	if (prevState.updatedKeyedFormData) {
	return {
	updatedKeyedFormData: false,
	};
	}
	const nextFormData = nextProps.formData;
	const previousKeyedFormData = prevState.keyedFormData;
	const newKeyedFormData =
	nextFormData == null || previousKeyedFormData == null
	? nextFormData
	: nextFormData.length === previousKeyedFormData.length
	? previousKeyedFormData.map((previousKeyedFormDatum, index) => {
	return {
	key: previousKeyedFormDatum.key,
	item: nextFormData[index],
	};
	})
	: generateKeyedFormData(nextFormData);
	return {
	keyedFormData: newKeyedFormData,
	};
	}

[epicfaace]

Ah, wait, I see that this PR (#2154) actually does exactly that! If you're good with it, I think we should merge that one instead of this one.

[richmahn]

@epicfaace I would not be hurt in the least if you did that. Haha

[ri0ter]

@epicfaace Yeah, just tested it and it seems to work.
It also kinda fixes #1346, kinda because in my opinion value null used with type ['array','null'] should display null instead of an empty array, but that might be more general issue with displaying nullables (or it's a feature).

Just as a side note, atm if I'll provide null value with type ['array'], it will display me an empty ArrayField, but at the same time I will see an error saying that it "should be an array". The problem is that visually it's already an array. So just as a future UI improvement, maybe if the value has wrong type it shouldn't display empty ArrayField but a special element which could be saying "initialize array".

[epicfaace]

Thanks for the explanation @ri0ter . I'll close this issue in favor of #2154, and close out some of these other related issues.

With the other PR, I did notice a minor issue with nullable arrays; I'm cataloguing it in #2212.

Just as a side note, atm if I'll provide null value with type ['array'], it will display me an empty ArrayField, but at the same time I will see an error saying that it "should be an array". The problem is that visually it's already an array. So just as a future UI improvement, maybe if the value has wrong type it shouldn't display empty ArrayField but a special element which could be saying "initialize array".

Could you make this into a separate issue so we could address it later?