Open brammitch opened 9 months ago
@brammitch It looks like very valuable PR, any plans merging it? cc @rreusser @nicolaskruchten
@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.
This would be a major upgrade. Can we get this merged?
@alexcjohnson would be nice to get this merged, is anyone able to look at it?
Thanks @brammitch and apologies for the delay - @archmoj would you be able to review this?
@brammitch great PR :medal_military:
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 great PR 🎖️ It looks like node
v18
is required. Is that right? Also in respect to these changes and switching to useplotly.js-dist-min
v2, I am wondering we should consider this a major change and publishreact-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.
Bumping @archmoj -- this would be helpful in my project :)
LGTM. Over to @alexcjohnson cc: @dmt0
As long as it's tested with RCE and CS, it's all good.
@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!
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.
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.
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.
It didn't for me.
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.
plotly.js-dist-min
v2tsup
vitest
and@testing-library/react
(88% coverage); replacesenzyme