Skip to content
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

ERROR in ./node_modules/image-size/lib/index.js #5377

Closed
bishonen opened this issue Jan 6, 2021 · 13 comments
Closed

ERROR in ./node_modules/image-size/lib/index.js #5377

bishonen opened this issue Jan 6, 2021 · 13 comments
Milestone

Comments

@bishonen
Copy link

bishonen commented Jan 6, 2021

Plotly.js version: 1.58.4
Platform: mac bis sur

When upgrading plotly from 1.52.2 to 1.58.4 a new error appears when trying to run it in a react webpage:

ERROR in ./node_modules/image-size/lib/index.js
Module not found: Error: Can't resolve 'fs' in '/local/home/xxx/node_modules/image-size/lib'
 @ ./node_modules/image-size/lib/index.js 3:9-22
 @ ./node_modules/plotly.js/src/traces/image/helpers.js
 @ ./node_modules/plotly.js/src/traces/image/calc.js
 @ ./node_modules/plotly.js/src/traces/image/index.js
 @ ./node_modules/plotly.js/lib/image.js
 @ ./node_modules/plotly.js/lib/index.js
 @ ./src/util/ui/MultiChart.js
 @ ./src/xxx/xxx.js
 @ ./src/App.js
 @ ./src/index.development.js

Is there some new dependency which needs to be added?

@archmoj
Copy link
Contributor

archmoj commented Jan 6, 2021

Duplicate of #5243, right?

@bishonen
Copy link
Author

bishonen commented Jan 6, 2021

Duplicate of #5243, right?

Yes, seems to be related - the descriptions are at least almost the same.
I lack the expertise to understand why image-size was moved to a dependency, tbh. Could perhaps someone more knowledgeable explain in a sentence how this affects using plotly in a react app? In the browser, there isn't any 'fs', of course, thus the whole thing has to fail. Is there something simple I'm missing here?

@archmoj archmoj added this to the v1.59.0 milestone Jan 6, 2021
@alexcjohnson
Copy link
Collaborator

I don't understand how image-size is (most of the time) avoiding looking for fs either. probe-image-size as discussed in #5243 certainly seems like a better option for us - it's explicitly meant for the browser, and we might even be able to do better on size by just bringing in the sync piece:

var sizeOf = require('probe-image-size/sync');

@rreusser
Copy link
Contributor

rreusser commented Jan 7, 2021

All of my initial guesses proved false. The dist version of image-size has require('fs'). I can't find that Plotly runs the brfs transform when bundling, which would browserify fs calls:

browserifyOpts.transform = [];

In short, it's not clear to me.

@antoinerg
Copy link
Contributor

FYI, I have no objection to replacing image-size with probe-image-size especially if we can only pick the sync part!

@archmoj
Copy link
Contributor

archmoj commented Jan 7, 2021

All of my initial guesses proved false. The dist version of image-size has require('fs'). I can't find that Plotly runs the brfs transform when bundling, which would browserify fs calls:

browserifyOpts.transform = [];

In short, it's not clear to me.

@rreusser simply FYI we are using image-size@0.7.5 not image-size@@0.9.3 which liked above.
Suggestions are welcome : )

@archmoj
Copy link
Contributor

archmoj commented Jan 8, 2021

Closing via #5388.

@archmoj archmoj closed this as completed Jan 8, 2021
@archmoj
Copy link
Contributor

archmoj commented Jan 8, 2021

@bishonen would you mind testing if the issue was resolved after #5388?

@bishonen
Copy link
Author

bishonen commented Jan 8, 2021

@archmoj of course! That's the least I can do :)
I'd test if after there's a new release, though, as I am not sure in what other way I could pull this change into a react app.

@archmoj
Copy link
Contributor

archmoj commented Jan 8, 2021

@bishonen in fact we are more interested to test this before next release which might take weeks.
Could you somehow load this latest build in your app?

@bishonen
Copy link
Author

bishonen commented Jan 9, 2021

@archmoj
I tried placing that build and simply replacing my plotly import in a react app to the file, but it fails to compile due to something else

Unexpected character '€' (4071:43)

@archmoj
Copy link
Contributor

archmoj commented Jan 9, 2021

Could you set charset to utf8?

@bishonen
Copy link
Author

bishonen commented Jan 9, 2021

Wasn't quite sure how you meant to change the encoding (in the header of the index.html ?). Therefore I searched, found and followed the advice from someone who already had this issue. They used the minified version which turned out to be working fine.

With this in place, I can gladly confirm that the latest build does not throw any 'fs' related errors anymore!

Compiled successfully.

Thanks to all for the quick turnaround. This is in part what makes plotly a great library (on which one can rely upon). Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants