-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Missing ndarray dependencies using plotly.js v2.5.0, npm v6 and react-plotly.js v1 #5925
Comments
What is your npm version? |
Locally it's 6.14.12. Our CI pipeline (GitHub Actions) would also be 6.x as we run on Node 14.x which I believe comes with 6.x npm as default. |
Upgrading to npm7 and node16 could help fix the warning. See #5922 and #5921 (comment). |
Do you no longer support node 14 then? Note that we get an error not a warning. |
node12 and node14 still worked for us with npm ci. The was a warning with npm i which upgrading to npm7 fixed it. |
I attached this above to the original report. |
Could you please try using ploly.js-dist instead? |
This appears to fix the issue. Are there any downsides to switching over to this package? Are you able to investigate what is causing the original problem? I see some of our other dependencies make use of the original ndarray rather than the plotly forked version. Do you think this could be the cause? |
There shouldn't be any downside. It should also download and build faster. In addition you may try the minified package plotly.js-dist-min too. |
Which other package require ndarray? |
FYI, switching to Node 16.x and NPM 7.x solves the error. We're not quite ready to switch to Node 16 yet in our project but we will keep in mind. For now maybe we just stay with Plotly 2.4.1 until we are ready to migrate. |
npm uninstall plotly.js Should fix the issue for you. |
If I uninstall plotly.js and then reinstall plot.js I now get a new error when running After running |
Not reinstalling plotly.js, npm install plotly.js-dist --save is installing ploltly.js-dist |
Yes I'm aware. We've already proved above that plotly.js-dist works just fine. I'm trying to help debug the issue with plotly.js and |
What's the reasoning behind not using ploly.js-dist then? |
I see. Please open an issue or pull request on react-plotly.js and paste the link here. |
Do I take it from your comments so far that you don't plan to try and debug and fix the issue with plotly.js 2.5.0, npm 6.x and |
Yes please you could continue using 2.4.1 (or bump to 2.4.2) until the react-plotly.js issue addressed or as discussed upgrade npm to v7. |
FWIW I can reproduce the error in a completely fresh project that has just plotly.js version 2.5.0 installed. Personally I would consider this a breaking change (or a bug) as you now no longer support npm 6.x and npm ci. How to reproduce:
|
Since v2 importing ploly.js-dist is the preferred and documented way of requiring, this may not be considered as a breaking change IMHO. |
Speaking as somebody who now has broken code, I beg to differ. The documentation for react-plotly clearly states to still install plotly.js. We clearly disagree here and that's fine. We can just stick to version 2.4.1 / 2.4.2 for our use case. |
@archmoj I understand your point but I think we should still make an effort to figure out what's going on here. If it's just a matter of npm6 not recognizing the link format we're using it'll be a simple fix. If it requires us to give all these forked packages different names ( BTW one thing I notice, not sure if this has anything to do with the error, but the |
@dmbarry86 FYI - I opened plotly/react-plotly.js#260. |
by installing "react-plotly.js": "git://github.com/plotly/react-plotly.js#40dfac18bd98d64b013bd726f5c212aef0d9e4d1" in |
Doesn't seem to be working for me. I get an error in our components that use react-plotly.js Package.json is now as follows I checked node_modules folder and it has successfully pulled down the version from your branch so I'm not sure exactly what is wrong here. |
@dmbarry86 thanks for testing. Could you possibly try "react-plotly.js": "git://github.com/plotly/react-plotly.js#4ecac06b1af7152f6cca7d1d0c577b9c594c9b9b" as well? |
@archmoj still the same problem with no source code inside node_modules\react-plotly.js folder. |
@archmoj I wonder if the problem is because the npmignore file in the repository is ignoring src folder. Maybe you could temporarily remove this and create new commit so we can test. https://github.com/plotly/react-plotly.js/blob/4ecac06b1af7152f6cca7d1d0c577b9c594c9b9b/.npmignore#L2 |
Unfortunately this week we may not be able to allocate more time investigating this problem. |
See my comment above regarding npmignore. I think for me to be able to npm install from GitHub and test your PR you will need to (temporarily) remove src from npmignore. |
Alternatively
|
@archmoj I've forked my own version of your branch and removed src from npmignore. I can now confirm that your PR is working as expected with npm version 6. |
This is a good article FWIW. It recommends the use of files in package.json rather than npmignore. https://medium.com/@jdxcode/for-the-love-of-god-dont-use-npmignore-f93c08909d8d |
@dmbarry86 Fantastic, please open a PR plotly/react-plotly.js@switch-to-plotly.js-dist...dmbarry86:switch-to-plotly.js-dist |
@archmoj It doesn't need a PR from me. That removal of src was just temporary to allow local testing. Your own PR plotly/react-plotly.js#260 is the one that should be merged. You should not merge the removal of src. |
Thanks for testing. Please leave a comment on this pull plotly/react-plotly.js#260. |
When reviewing our weekly Dependabot updates I came across an issue with our migration from Plotly 2.4.1 to 2.5.0. We now get the error shown below when trying to install dependencies. The issue was shown in our CI pipeline which runs
npm ci
.I've attached our package files from 2.4.1 which work fine and the updated ones from 2.5.0 which break. You can run
npm ci
with the 2.5.0 files to reproduce.error.zip
The text was updated successfully, but these errors were encountered: