Text Label props

[Avd6977]

Expected Behavior

Passing in text label props to use defaults if not overridden

Current Behavior

Passing custom text label props that don't include the default breaks loading
e.g.

textLabels: {
	body: {
		noMatch: this.props.emptyMessage
	}
}

Steps to Reproduce (for bugs)

Pass in the prop above

Your Environment

Tech	Version
MUI-datatables	2.9.0

[gabrielliwerant]

I wasn't able to reproduce the issue with the information here. I loaded the customize-search example from v2.10.0 and then modified the options:

const options = {
  // ... a bunch of options
  textLabels: {
    body: {
      noMatch: 'empty message'
    }
  }
}

Nothing breaks for me when I do that.

[nhattextiq]

I had this problem today as well. When I removed the textLabels part in my options, everything would work.

  const finalOptions = (
      Object.assign({},
        DEFAULT_OPTIONS,
        { textLabels: {
          body: {
            noMatch: isLoading
              ? <CircularLoading classes={classes} />
              : intl.formatMessage({ id: 'noRecord' })
          }
        } },
        otherOptions
      )
  )

Stack trace is as follow:

(t, ) TypeError: Cannot read property 'search' of undefined

Stack trace:
TypeError: Cannot read property 'search' of undefined
at t.value (webpack:///./node_modules/mui-datatables/dist/index.js?:1:45681)
at t.componentRender [as render] (webpack:///./node_modules/react-hot-loader/dist/react-hot-loader.development.js?:2243:64)
at finishClassComponent (webpack:///./node_modules/@hot-loader/react-dom/cjs/react-dom.development.js?:17036:31)
at updateClassComponent (webpack:///./node_modules/@hot-loader/react-dom/cjs/react-dom.development.js?:16991:24)
at beginWork$1 (webpack:///./node_modules/@hot-loader/react-dom/cjs/react-dom.development.js?:18502:16)
at beginWork$$1 (webpack:///./node_modules/@hot-loader/react-dom/cjs/react-dom.development.js?:23190:14)
at performUnitOfWork (webpack:///./node_modules/@hot-loader/react-dom/cjs/react-dom.development.js?:22205:12)
at workLoopSync (webpack:///./node_modules/@hot-loader/react-dom/cjs/react-dom.development.js?:22182:22)
at renderRoot (webpack:///./node_modules/@hot-loader/react-dom/cjs/react-dom.development.js?:21875:11)
at runRootCallback (webpack:///./node_modules/@hot-loader/react-dom/cjs/react-dom.development.js?:21551:20)

I tried with just noMatch: 'empty message' and it still throws that error.
This is with version 2.10.0

[patorjk]

Yep, this is a bug - it's a tricky one though. Check out this CodeSandbox:

https://codesandbox.io/s/muidatatables-custom-toolbar-5gwgy

To see the bug, hit the "loading" toggle. It looks like it's possibly related to more than textLabels, but if you comment "textLabels" out of the options, it doesn't appear (which is probably why I don't see it in my app - I don't use that option).

Checking the versions, it looks like it was introduced in version 2.8.1.

Side note: I'm running low on Sandboxes, so after this issue is resolved I'll probably deleted that sandbox.

[patorjk]

So this bug has subtle affects elsewhere. If you set selectableRows to "false" in your options. The table will initially change it to "none" for you and print a warning in the console. However, once the state updates and the table re-renders with new props, this value gets reset back to "false" and the table will print an error in the console and the checkboxes will become visible.

This issue looks like it was introduced when merge was changed to assign 2.8.1:

432ae09

I'd have to better understand why the change was made, but I think this can be resolved by using "mergeWith" and selectively merging from the newProp object. Below is something I played around with which seems to work - but I'm not totally familiar with why the change was made so I can't say for certain if this would fix everything. It does fix the above textLabels and selectableRows issues though.

import mergeWith from 'lodash.mergewith';

...

  updateOptions(props) {
    this.options = mergeWith(this.options, props.options, function(currentProp, newProp) {
      // these are just for debug, so you can see what's happening
      console.log('prop'); 
      console.dir(currentProp);
      console.dir(newProp);

      if (Array.isArray(currentProp) || currentProp === null) {
        return newProp;
      }

      // selectableRows API change
      if (typeof currentProp === 'string' && typeof newProp === 'boolean') {
        if (currentProp === 'multiple' || currentProp === 'none') {
          return currentProp; // this is a previously set selectableRows option
        }
      }

      switch (typeof currentProp) {
        case 'boolean':
        case 'number':
        case 'string':
        case 'function':
        case 'bigint':
        case 'symbol':
          return newProp;
      }
      return undefined; // let merge recursively handle object and undefined types
    });
  }

[gabrielliwerant]

This is kind of a big bug, so thanks for the detective work @patorjk and for opening the issue @Avd6977. I needed to see the exact situation that leads to the issue, which is when the table updates.

We can't use merge because that results in a different bug. It basically breaks any customization of selectable rows, and probably other things, because in that situation, when the table updates, we need the table to overwrite the new state with the props, and merge will keep defaults instead. However, using assign has an unintended consequence where it overwrites whole objects instead of just the defined nodes (we'd want it to supply all the unsupplied nodes of the textLabels while overwriting any of the old ones with the new ones that happen to be supplied).

Working on a fix as we speak, and will issue a patch when done.

[patorjk]

Sounds good - you may want to try out mergeWith like in my example above. It’s like merge, but it allows you to override how merge works. Above I have it act like assign unless the property type is object or undefined.

I think it gives the desired behavior but am not 100% sure.

[gabrielliwerant]

@patorjk I think I was able to get there with assignWith, but it's a similar idea, so thanks for the tip. The PR is up if you want to take a look and let me know what you think! I'll merge it later tomorrow (Monday) if you can't get to it by then, so we don't go too long without a getting a fix in.

[patorjk]

@gabrielliwerant Sure, I'll check it out here in a few minutes. If I have any comments I'll post them on the PR.

[Avd6977]

Thanks @patorjk and @gabrielliwerant. Been away all weekend so haven't had the chance to do the demo/explain.