Skip to content

Fix browser module resolution#614

Merged
Stuk merged 1 commit intoStuk:masterfrom
mgechev:fix-browser-resolution
Apr 1, 2020
Merged

Fix browser module resolution#614
Stuk merged 1 commit intoStuk:masterfrom
mgechev:fix-browser-resolution

Conversation

@mgechev
Copy link
Copy Markdown
Contributor

@mgechev mgechev commented Aug 21, 2019

When webpack/rollup or another bundler, implementing the
browser-resolve
spec tries to resolve jszip for a browser-based project they fail
with:

ERROR in ./node_modules/jszip/lib/readable-stream-browser.js
Module not found: Error: Can't resolve 'stream' in '/node_modules/jszip/lib'

Since you already produce a browser build, we just need to point to
it inside of package.json's browser field.

PS: You'd probably want to double check the semantics of ".".

Looking at browser-resolve it seems that everything will work
as expected. Also, trying empirically with webpack, everything worked
out, but few more pairs of eyes would be greatly appreciated.

Also @sokra should be able to share a valuable opinion.

Fix #524
Fix #521
Fix #477

When webpack/rollup or another bundler, implementing the
[`browser-resolve`](https://www.npmjs.com/package/browser-resolve)
spec tries to resolve `jszip` for a browser-based project they fail
with:

```
ERROR in ./node_modules/jszip/lib/readable-stream-browser.js
Module not found: Error: Can't resolve 'stream' in '/node_modules/jszip/lib'
```

Since you already produce a browser build, we just need to point to
it inside of `package.json`'s `browser` field.

PS: You'd probably want to double check the semantics of `"."`.

Looking at `browser-resolve` it seems that everything will work
as expected. Also, trying empirically with webpack, everything worked
out, but few more pairs of eyes would be greatly appreciated.

Also @sokra should be able to share a valuable opinion.

Fix Stuk#524
Fix Stuk#521
Fix Stuk#477
@mgechev
Copy link
Copy Markdown
Contributor Author

mgechev commented Aug 21, 2019

@Stuk the build seems to be timing out. Doesn't look related to the change.

@hakimio
Copy link
Copy Markdown

hakimio commented Sep 9, 2019

@Stuk can you merge this, so we could build Angular 8 apps using jszip?

@Stuk
Copy link
Copy Markdown
Owner

Stuk commented Sep 13, 2019

Thanks for the PR! I just tried bundling JSZip with webpack and parcel and had no issues.

This is my testing package.json:

{
  "name": "jszip-package-test",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "",
  "license": "ISC",
  "dependencies": {
    "jszip": "^3.2.2",
    "parcel-bundler": "^1.12.3",
    "webpack": "^4.39.3",
    "webpack-cli": "^3.3.8"
  }
}

Both ./node_modules/.bin/webpack index.js and ./node_modules/.bin/parcel build index.js completed successfully.

Could you create a package.json, or repository that reproduces the issue?

@mgechev
Copy link
Copy Markdown
Contributor Author

mgechev commented Sep 13, 2019

Here it is. Run:

npm i && npm run build

@Stuk
Copy link
Copy Markdown
Owner

Stuk commented Sep 15, 2019

Thanks! I can repro. I need to understand the impact of this PR on Webpack/Browserify/Parcel etc. bundles.

I think it might increase bundle sizes for folks, because there could be 2 stream implementations in the bundle: 1 in the JSZip dist, and another one added by the bundler.

If you're able to help me understand the impact, I'd very much appreciate it.

@eduboxgithub
Copy link
Copy Markdown

I'm using Angular 8 and the latest version of jszip and I'm seeing the same error when I build my project:

Module not found: Error: Can't resolve 'stream'

If the error cames from the file deadable-stream-browser.js

/*
 * This file is used by module bundlers (browserify/webpack/etc) when
 * including a stream implementation. We use "readable-stream" to get a
 * consistent behavior between nodejs versions but bundlers often have a shim
 * for "stream". Using this shim greatly improve the compatibility and greatly
 * reduce the final size of the bundle (only one stream implementation, not
 * two).
 */
module.exports = require("stream");

@alejandrocoding
Copy link
Copy Markdown

Hi @Stuk is there any news on this? is it possible to merge this PR?
I'm having error in the CI and with the angular build.
Thanks!

@Stuk
Copy link
Copy Markdown
Owner

Stuk commented Apr 1, 2020

Just spent some time checking that this doesn't adversely affect webpack or parcel, and it looks like it's ok. Tested here

@Stuk Stuk merged commit bfbd255 into Stuk:master Apr 1, 2020
@Stuk
Copy link
Copy Markdown
Owner

Stuk commented Apr 1, 2020

Published as v3.3.0

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can't resolve 'stream' How can I use jszip with Expo (React Native) ? "Error: cannot find module 'stream'" when using Chrome

5 participants