Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect initial values for objects with array fields #1112

Closed
wight554 opened this issue Apr 29, 2022 · 5 comments · Fixed by #1123
Closed

Incorrect initial values for objects with array fields #1112

wight554 opened this issue Apr 29, 2022 · 5 comments · Fixed by #1123
Assignees
Labels
Type: Bug Bug reports and their fixes
Milestone

Comments

@wight554
Copy link

When using array field (even with default value) I'm getting this:

Uncaught TypeError: parent.value is undefined
    ListAdd ListAddField.js:12
    React 4
ListAddField.js:12

Compiled module code:

import { __rest } from "tslib";
import FormControl from '@material-ui/core/FormControl';
import IconButton from '@material-ui/core/IconButton';
import cloneDeep from 'lodash/cloneDeep';
import React from 'react';
import { connectField, filterDOMProps, joinName, useField, } from 'uniforms';
function ListAdd(_a) {
    var { disabled, fullWidth = true, icon = '+', initialCount, margin = 'dense', name, readOnly, value, variant } = _a, props = __rest(_a, ["disabled", "fullWidth", "icon", "initialCount", "margin", "name", "readOnly", "value", "variant"]);
    const nameParts = joinName(null, name);
    const parentName = joinName(nameParts.slice(0, -1));
    const parent = useField(parentName, { initialCount }, { absoluteName: true })[0];
    const limitNotReached = !disabled && !(parent.maxCount <= parent.value.length);
    return (React.createElement(FormControl, { fullWidth: fullWidth, margin: margin, variant: variant },
        React.createElement(IconButton, Object.assign({}, filterDOMProps(props), { disabled: !limitNotReached, onClick: () => {
                if (!readOnly) {
                    parent.onChange(parent.value.concat([cloneDeep(value)]));
                }
            } }), icon)));
}
export default connectField(ListAdd, {
    initialValue: false,
    kind: 'leaf',
});

Easiest workaround would be not to use it, here you can find possible replacements:
https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/no-non-null-assertion.md

For example:

  const limitNotReached = !disabled && !(parent?.minCount !== undefined && value !== parent?.undefined && parent?.minCount >= parent?.value.length);
@radekmie radekmie self-assigned this Apr 29, 2022
@radekmie radekmie added the Type: Bug Bug reports and their fixes label Apr 29, 2022
@radekmie radekmie added this to the v3.9 milestone Apr 29, 2022
@radekmie
Copy link
Contributor

Hi @wight554. We're aware of how the non-null assertion work, and it's being used deliberately in there. And while we could change this code, it should never be the case, that your ListAddField has a parent without a value. Could you show us, what does trigger this error?

@wight554
Copy link
Author

Hi @wight554. We're aware of how the non-null assertion work, and it's being used deliberately in there. And while we could change this code, it should never be the case, that your ListAddField has a parent without a value. Could you show us, what does trigger this error?

Sure, schema is:

new SimpleSchema2Bridge(
  new SimpleSchema({
    string1: {
      type: String,
      label: 'Primary Objective',
    },
    object1: { type: Object, label: '' },
    'object1.boolean1': { type: Boolean, label: 'label 1' },
    'object1.array1': {
      type: Array,
      minCount: 1,
      defaultValue:[{}],
    },
    'object1.array1.$': { type: Object },
    'object1.array1.$.string1': { type: String },
    'object1.array1.$.string2': { type: String },
    'object1.array1.$.string3': { type: String },
  })
)

Easy to check, it breaks playground as well:
TypeError: m.value is undefined

@radekmie
Copy link
Contributor

Thank you! I've stripped it down a little and here's a minimal reproduction:

new SimpleSchema2Bridge(
  new SimpleSchema({
    x: { type: Object },
    'x.y': { type: Array },
    'x.y.$': { type: String },
  })
)

@wight554
Copy link
Author

wight554 commented May 4, 2022

seems like this schema might also break SelectField with multiple (at least with mui5):

Uncaught Error: MUI: The `value` prop must be an array when using the `Select` component with `multiple`.
    node_modules vendors-node_modules_date-io_date-fns_build_index_esm_js-node_modules_mui_icons-material_Add_-5b00d7.chunk.js:27657
    SelectInput2 SelectInput.js:359
    React 12
    unstable_runWithPriority scheduler.development.js:468
    React 10
    unstable_runWithPriority scheduler.development.js:468
    React 15
    tsx bootstrap.tsx:32
    factory react refresh:6
    Webpack 7
SelectInput.js:374

default value for array is undefined, would be better to have it as empty array

@radekmie
Copy link
Contributor

radekmie commented May 7, 2022

I've dug into it, and the problem is more complicated than I thought. First of all, the default (initial) value of x.y is [], so this one is fine. However, the order in which they're set is not.

Using the schema I posted in #1112 (comment), we see these onChange calls:

{ key: 'x.y', value: [] }
{ key: 'x.y', value: [] }
{ key: 'x', value: {} }

(The x.y one is doubled, as one comes from the NestField, and the second from ListAddField.)

Of course, that's the expected order, because of the way how useEffect works. At the same time, this is NOT the wanted one. There are a few options then:

  1. Provide an explicit default of x (object1 in your schema), like this: defaultValue: { y: [] } ({ array1: [] } in your schema). This will work straight away, with the current code (playground).
  2. Improve the getInitialValue of... Well, all bridges. Right now, all of them return empty objects (here's an example). It could look like this:
         if (field.type === Object || field.type instanceof SimpleSchema) {
    -      return {};
    +      const value: Record<string, unknown> = {};
    +      this.getSubfields(name).forEach(key => {
    +        value[key] = this.getInitialValue(joinName(name, key));
    +      });
    +      return value;
         }
    It fixes this problem, but still, two unnecessary onChange calls happen.

To sum up, 1. is a workaround for the time being, if you need one. I'll think whether we can implement 2. in v3, as it's techincally a breaking change. If not, it'll definitely be there in v4. (I actually have a rework of the initial model logic in mind for a while.)

@radekmie radekmie changed the title non-null assertions are lost in esm compiled modules Incorrect initial values for objects with array fields Jun 6, 2022
@radekmie radekmie modified the milestones: v3.9, v3.10 Jun 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Bug reports and their fixes
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants