Modernize react-plotly.js and support plotly.js-dist-min v2

[brammitch]

• Rewrite library (in TypeScript) with hooks; supports React 18
• Support plotly.js-dist-min v2
• Output both CJS and ESM with tsup
• All existing tests passing with vitest and @testing-library/react (88% coverage); replaces enzyme
• Update dependencies and use modern config settings to reduce sprawl (package-lock.json went from 29k -> 11k lines)

[sirpeas]

@brammitch It looks like very valuable PR, any plans merging it?
cc @rreusser @nicolaskruchten

[brammitch]

@brammitch It looks like very valuable PR, any plans merging it? cc @rreusser @nicolaskruchten

I hope so!

If @alexcjohnson or any other maintainer wants to review this PR, I'd be happy to discuss further changes, rework, or testing.

[mkilp]

This would be a major upgrade. Can we get this merged?

[kabirgh]

@alexcjohnson would be nice to get this merged, is anyone able to look at it?

[alexcjohnson]

Thanks @brammitch and apologies for the delay - @archmoj would you be able to review this?

[archmoj]

@brammitch great PR 🎖️
It looks like node v18 is required. Is that right?
Also in respect to these changes and switching to use plotly.js-dist-min v2, I am wondering we should consider this a major change and publish react-plotly.js v3 thanks to your PR labeled 333!
What do you think?

On another note - please use npm audit fix to fix one remaining vulnerability in the package-lock.

[brammitch]

@brammitch great PR 🎖️ It looks like node v18 is required. Is that right? Also in respect to these changes and switching to use plotly.js-dist-min v2, I am wondering we should consider this a major change and publish react-plotly.js v3 thanks to your PR labeled 333! What do you think?

On another note - please use npm audit fix to fix one remaining vulnerability in the package-lock.

Thanks for the feedback @archmoj!

The vulnerability has been resolved.

I updated the package.json with your recommendations, after verifying that node v18 is required:

  "name": "react-plotly.js",
  "version": "3.0.0",
  "engines": {
    "node": ">= 18"
  },

I also updated the README references to plotly.js, replacing them with plotly.js-dist-min. Probably more could be done to update the docs, like adding some examples with React functional components, but that could be another PR.

[kabirgh]

Bumping @archmoj -- this would be helpful in my project :)

[archmoj]

LGTM.
Over to @alexcjohnson
cc: @dmt0

[dmt0]

As long as it's tested with RCE and CS, it's all good.

[alexcjohnson]

@dmt0 you may feel differently, but personally I wouldn't worry about CS prior to publishing this. Testing with RCE should be sufficient to surface most potential issues with CS, then if any issues were to arise in CS we could either hold it to v2.x or address them from the CS side.

@archmoj @dmt0 I'll leave that testing to you two, this looks good from my side. Thanks again @brammitch!

[dmt0]

Unfortunately react-chart-editor won't run. Getting this on start:

react.development.js:209 Warning: Invalid hook call. Hooks can only be called inside of the body of a function component. This could happen for one of the following reasons:
1. You might have mismatching versions of React and the renderer (such as React DOM)
2. You might be breaking the Rules of Hooks
3. You might have more than one copy of React in the same app
See https://reactjs.org/link/invalid-hook-call for tips about how to debug and fix this problem.

printWarning @ react.development.js:209
react.development.js:1622 Uncaught TypeError: Cannot read properties of null (reading 'useState')
    at useState (react.development.js:1622:1)
    at PlotlyComponent (factory.mjs:58:33)
    at renderWithHooks (react-dom.development.js:14804:1)
    at mountIndeterminateComponent (react-dom.development.js:17483:1)
    at beginWork (react-dom.development.js:18597:1)
    at HTMLUnknownElement.callCallback (react-dom.development.js:189:1)
    at Object.invokeGuardedCallbackDev (react-dom.development.js:238:1)
    at invokeGuardedCallback (react-dom.development.js:293:1)
    at beginWork$1 (react-dom.development.js:23204:1)
    at performUnitOfWork (react-dom.development.js:22155:1)

react-dom.development.js:19528 The above error occurred in the <PlotlyComponent> component:
    in PlotlyComponent (created by PlotlyEditor)
    in div (created by PlotlyEditor)
    in div (created by PlotlyEditor)
    in PlotlyEditor (created by ForwardRef)
    in ForwardRef (created by App)
    in div (created by App)
    in App (created by HotExportedApp)
    in AppContainer (created by HotExportedApp)
    in HotExportedApp

React will try to recreate this component tree from scratch using the error boundary you provided, AppContainer.

[brammitch]

I'm looking at it now. RCE has a hard dependency on react 16.14.0. When setting the RCE package.json to use my local version of react-plotly.js, it's installing react 18 to satisfy @testing-library/react, resulting in two different versions of react being installed.

[brammitch]

Made some progress, but still don't have RCE working.

In local react-plotly.js directory, run npm build and then npm pack. This will create react-plotly.js-3.0.0.tgz.

In local react-chart-editor directory, install the pack we just generated (e.g., npm i ../react-plotly.js/react-plotly.js-3.0.0.tgz --legacy-peer-deps --save).

Checking npm ls react now shows only one version of react since npm is no longer trying to install all the react-plotly.js devDependencies.

But now there's a webpack error when running npm run watch, because it can't handle the jsx-transform. Adding a line to the RCE webpack.config.js fixes it:

  resolve: {
    alias: {
      'react-dom': '@hot-loader/react-dom',
      'react/jsx-runtime': require.resolve('react/jsx-runtime'),
    },
  },

Now RCE will build, storybook works, all tests pass, but npm run watch causes the browser to hang and nothing is rendered on the page for me. I didn't know anything about RCE for today, so I'm not sure where to go from here.

[archmoj]

Does RCE work after 500a343?

[brammitch]

It didn't for me.

[brammitch]

I believe that this useEffect is what iscausing the problem with RCE.

[brammitch]

The original componentDidUpdate function compared frames, layout, data, config, and revision.

I did not code to that specific logic because I expected useEffect to handle it. When RCE starts, the updatePlotly callback is called over and over again. I tried implementing the componentDidUpdate logic in the useEffect:

    // componentDidUpdate
    useEffect(() => {
      let shouldUpdate = true;

      // frames *always* changes identity so fall back to check length only :(
      const numPrevFrames = prevProps?.frames?.length ?? 0;
      const numNextFrames = props.frames?.length ?? 0;

      const dataChanged = !isEqual(prevData, data);
      const layoutChanged = !isEqual(prevProps?.layout, props.layout);
      const configChanged = !isEqual(prevProps?.config, props.config);
      const numFramesChanged = numNextFrames !== numPrevFrames;

      const figureChanged = dataChanged || layoutChanged || configChanged || numFramesChanged;

      const revisionDefined = prevProps?.revision !== void 0;
      const revisionChanged = prevProps?.revision !== props.revision;

      if (!figureChanged && (!revisionDefined || (revisionDefined && !revisionChanged))) {
        shouldUpdate = false;
      }

      if (shouldUpdate && props.onUpdate) {
        updatePlotly(props.onUpdate);
      }
    }, [data, prevData, prevProps, props, updatePlotly]);

but this causes RCE to error due to "too many re-renders".

[brammitch]

I would still like to revisit this and dive deeper into the issues my PR has with RCE, but my day job is keeping my busy at the moment. If anyone else has cycles to improve this PR to get it compatible with RCE, I'd welcome the collaboration.