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

fix(JSZipDataAccessHelper): Downgrade JSZip version #1651

Merged
merged 1 commit into from Dec 14, 2020

Conversation

boucaud
Copy link
Contributor

@boucaud boucaud commented Dec 9, 2020

The latest versions of JSZip seem to have broken the JSZipDataAccessHelper.

Calling JSRoot.file().async() returns a promise that never resolves. This breaks .vtkjs loading functionality.

People having the same issue:

Stuk/jszip#724

Stuk/jszip#712

Downgrading to 3.2.0 fixes the issue.

@boucaud
Copy link
Contributor Author

boucaud commented Dec 9, 2020

@floryst What do you think ?

@finetjul @laurentmalka fyi.

@jourdain
Copy link
Collaborator

jourdain commented Dec 9, 2020

Do we need stream as an added dependency?

@finetjul finetjul requested a review from floryst December 9, 2020 16:04
@boucaud
Copy link
Contributor Author

boucaud commented Dec 9, 2020

Do we need stream as an added dependency?

Without it we get this error:

ERROR in ../node_modules/jszip/lib/readable-stream-browser.js 9:0-34
Module not found: Error: Can't resolve 'stream' in '[...]/vtk-js/node_modules/jszip/lib'

@floryst
Copy link
Collaborator

floryst commented Dec 9, 2020

That should have been covered by the stream-browserify dependency I added, but I guess webpack isn't picking up on it. I did a quick test and the following webpack.common.js patch should work without having to pull in the extra stream dependency.

diff --git a/webpack.common.js b/webpack.common.js
index 44dd277ee5..5b2dd6aa8c 100644
--- a/webpack.common.js
+++ b/webpack.common.js
@@ -97,6 +97,7 @@ const baseConfig = {
     alias: {
       'vtk.js': __dirname,
     },
+    fallback: { "stream": require.resolve("stream-browserify") },
   },
   module: {
     rules: configureVtkRules(),
@@ -148,6 +149,7 @@ const liteConfig = merge(
           'Sources/Rendering/Core/ColorTransferFunction/LiteColorMaps.json'
         ),
       },
+      fallback: { "stream": require.resolve("stream-browserify") },
     },
   },
   baseConfig,

package.json Outdated Show resolved Hide resolved
@boucaud boucaud force-pushed the fix-jszip-examples branch 2 times, most recently from 040fc7f to 0ad7147 Compare December 10, 2020 14:57
@floryst floryst merged commit 0f845fa into Kitware:master Dec 14, 2020
@jourdain
Copy link
Collaborator

🎉 This PR is included in version 15.3.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

@jourdain jourdain added the released Automated label label Dec 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Automated label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants