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 browser module resolution #614

Merged
merged 1 commit into from
Apr 1, 2020
Merged

Conversation

mgechev
Copy link
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
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

hakimio commented Sep 9, 2019

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

@Stuk
Copy link
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
Contributor Author

mgechev commented Sep 13, 2019

Here it is. Run:

npm i && npm run build

@Stuk
Copy link
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

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

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
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
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
5 participants