[PF4] lodash-es problems in React-table

[aljesusg]

Hi

After add the react-table for the VirtualScroll in kiali we have a problem with lodash-es library imported in react-table.

Why react-table use lodash-es instead lodash like react-charts?

  This usually means that you are trying to import a file which Jest cannot parse, e.g. it's not plain JavaScript.

    By default, if Jest sees a Babel config, it will use that to transform your files, ignoring "node_modules".

    Here's what you can do:
     • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
     • If you need a custom transformation specify a "transform" option in your config.
     • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.

    You'll find more details and examples of these config options in the docs:
    https://jestjs.io/docs/en/configuration.html

    Details:

    /home/aljesusg/Projects/kiali/kiali-ui/node_modules/@patternfly/react-table/node_modules/lodash-es/lodash.js:10
    export { default as add } from './add.js';
    ^^^^^^

    SyntaxError: Unexpected token export

      at ScriptTransformer._transformAndBuildScript (node_modules/@jest/transform/build/ScriptTransformer.js:471:17)
      at ScriptTransformer.transform (node_modules/@jest/transform/build/ScriptTransformer.js:513:25)
      at Object.<anonymous> (node_modules/@patternfly/react-table/dist/js/components/Table/base/evaluate-transforms.js:8:17)

Thanks

[karelhala]

@redallen any reason why lodash-es was chosen in favor of lodash?

We have encountered similiar problem when running tests with jest and found workaround by aupdating moduleNameMapper:

"moduleNameMapper": {
  "\\.(css|scss)$": "identity-obj-proxy",
  "^lodash-es$": "lodash"
}

It has no impact on bundle size and effect tests only.

[aljesusg]

For us moduleNameMapper is not supported by create react app :(

Out of the box, Create React App only supports overriding these Jest options:

  • collectCoverageFrom
  • coverageReporters
  • coverageThreshold
  • extraGlobals
  • globalSetup
  • globalTeardown
  • resetMocks
  • resetModules
  • snapshotSerializers
  • watchPathIgnorePatterns.

These options in your package.json Jest configuration are not currently supported by Create React App:

  • moduleNameMapper

If you wish to override other Jest options, you need to eject from the default setup. You can do so by running npm run eject but remember that this is a one-way operation. You may also file an issue with Create React App to discuss supporting more options out of the box.

Maybe with watchPathIgnorePatterns

[rachael-phillips]

@priley86 making you aware.

[redallen]

React table depends on lodash-es. It used to be a peer dependency, but to resolve #2222 it was moved to being a dev dependency.

As for the difference between lodash and lodash-es, lodash-es produces smaller bundle sizes because it uses ESM exports instead of CommonJS. I agree as a library we should consistently only use lodash-es. I've opened #2617 for this.

Is @karelhala 's workaround sufficient for you?

[dlabrecq]

"I used lodash-es instead of lodash because it is currently a dependency in OpenShift." -- @priley86

Victory charts has lodash as a dependency; thus, react-charts inherits this dependency as well. It's not likely that we can change that to use lodash-es.

[priley86]

I am seeing that babel-plugin-lodash and lodash-webpack-plugin can be used to treeshake lodash imports like import foo from 'lodash/foo', but I don't see why we would prefer this option over lodash-es for the reason @redallen mentions (it's already ESM).

For PF, I am perfectly fine w/ swaying one way here. For some products, this may become very hard due to the 3rd party ecosystem. For that reason, I like the ideas introduced here:
openshift/console#1724

That seems overkill for PF though.

cc: @vojtechszocs

[karelhala]

@aljesusg you should be able to pass additional attributes directly to react-scripts [1], so you might want to fix it with this.

but I don't see why we would prefer this option over lodash-es for the reason @redallen mentions (it's already ESM)

I think that using babel-plugin-lodash would be better approach. It plays nicely with jest, so if we'd were to use it and change all lodashes to lodash instead of lodash-es we wouldn't force consumers to fight these problems.

[1] facebook/create-react-app#2474

[aljesusg]

@karelhala I tried to pass --transformignore.... But nothing, we are not using Babel :(

[priley86]

hi, I spoke to @redallen and @vojtechszocs some further. I think it is probably best to convert react-table to use lodash instead of lodash-es to avoid this potential problem for the time being. Then consumers using legacy format can just use lodash and babel-plugin-lodash if using babel:

import times from 'lodash/times'

There is a good article describing performance gains w/ babel here.

For typescript consumers, i believe this would just be lodash, @types/lodash, @types/lodash-es (option 2 in the guide here).

import get from 'lodash/get'

This is somewhat controversial still, but it seems the easier path if other dependencies will continue using lodash.