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

Browser version requires missing files. #694

Closed
wallabra opened this issue Jan 16, 2019 · 15 comments
Closed

Browser version requires missing files. #694

wallabra opened this issue Jan 16, 2019 · 15 comments
Labels
bug there is a bug in the way jimp behaves

Comments

@wallabra
Copy link
Contributor

Expected Behavior

Using the browser version of JIMP, e.g. when running Browserify on a Node.JS package that requires JIMP, should be clean, as it should contain all of the JIMP code in a single file.

Current Behavior

Browserify (most likely the browser too, if you use the browser version directly in the browser) will error due to erroneous calls to require().

Failure Information (for bugs)

The error complains about ./src/converter, which simply doesn't exist in jimp/browser (where jimp is the node_modules subfolder where jimp is installed, when installed from npm).

Steps to Reproduce

I haven't tried to use JIMP directly in the browser, but here's how to reproduce this error the way I noticed it.

  1. Create a simple Node.JS project.
  2. Install JIMP as a dependency (npm i --save jimp).
  3. Create an index.js file with the following contents:
const JIMP = require('jimp');
  1. Run browserify index.js.

Context

  • Jimp Version: JIMP 0.6.0.
  • Operating System: Linux Mint 19 "Tara" (Ubuntu fork), kernel Linux v4.20.
  • Node version: Node.JS v8.10.0.

Failure Logs

> nodemips@0.1.1 build /home/gustavo6046/Source code/nodemips
> browserify -r ./index.js:mips | babel --minified -f minif.tmp | uglifyjs >browser/mips.js

Error: Cannot find module './src/converter' from '/home/gustavo6046/Source code/nodemips/node_modules/jimp/browser/lib'
    at /home/gustavo6046/Source code/nodemips/node_modules/browser-resolve/node_modules/resolve/lib/async.js:55:21
    at load (/home/gustavo6046/Source code/nodemips/node_modules/browser-resolve/node_modules/resolve/lib/async.js:69:43)
    at onex (/home/gustavo6046/Source code/nodemips/node_modules/browser-resolve/node_modules/resolve/lib/async.js:92:31)
    at /home/gustavo6046/Source code/nodemips/node_modules/browser-resolve/node_modules/resolve/lib/async.js:22:47
    at FSReqWrap.oncomplete (fs.js:152:21)
@wallabra
Copy link
Contributor Author

I've noticed that simply removing the browser field from JIMP's package.json seems to fix the issue (although I haven't tested the output in a browser environment yet).

@hipstersmoothie
Copy link
Collaborator

Do you think should be browserifying itself? Or should it be left to users

@wallabra
Copy link
Contributor Author

Pardon me, I'm not sure if I get your question; what should be Browserifying itself?

@hipstersmoothie
Copy link
Collaborator

Jimp already tries to make a browserified version of itself (thats what isn't working for you). My question is who should the browserify step belong to, me or you?

I didn't add that functionality so I'm not sure how useful or necessary it is

@wallabra
Copy link
Contributor Author

I'm pretty sure it would be better to leave it to the user, unless JIMP relies on features that don't work in the browser.

@hipstersmoothie hipstersmoothie added the bug there is a bug in the way jimp behaves label Sep 3, 2019
@caldwellc
Copy link

I agree with @Gustavo6046. I think it would be better left to the user as well. Any updates regarding this issue?

@wallabra
Copy link
Contributor Author

wallabra commented Jul 9, 2020

Yeah, I'm interested too! JIMP is really cool, but kind of obscure, and this could actually be pretty helpful in that regard.

@hipstersmoothie
Copy link
Collaborator

This just requires removing the browserify bundle from npm right? We would still need to use it to test in a browser (I think).

More than happy to accept a PR for this

@wallabra
Copy link
Contributor Author

I can do it if you want me to. It'll be quick and simple enough.

Either way, maybe it would be a good idea to assign someone to do the job, to avoid concurrent pull requests trying to do the same thing.

@hipstersmoothie
Copy link
Collaborator

Go for it!

@danielbarela
Copy link

@Gustavo6046 any updates on this?

@wallabra
Copy link
Contributor Author

Ah yes, sorry. Been busy lately, but I can now. Give me a moment.

@wallabra
Copy link
Contributor Author

I cloned it, and it seems that the browser field does not exist in package.json anymore, which leads me to believe that this issue might already have been solved in a commit since it was first put here. I will quickly test it, though.

@wallabra
Copy link
Contributor Author

Oops! That was just the top level package.json in the repo. Totally overlooked packages! My bad!

@wallabra
Copy link
Contributor Author

Yep, that seems to work so far. I think Browserify is supposed to handle the "browserification" itself (hence the name), and a library explicitly concerning with Browserify compatibility will break stuff. Instead, we should only concern with building a single JS output for the web, and allow that Browserify do that itself if Jimp is linked as a npm dependency rather than a script tag in a webpage somewhere.

To this end, all that is needed is to remove the browser field from package.json, as package.json is only ever used by npm (and thus Browserify), and thus there is no point in including it. To build for including with a script tag, use a development environment, that is, use Browserify as a npm script, and distribute the output .js file for browser usage (e.g. using a CDN or a home website). Browsers don't use npm!

Anyway, I have changed this one single line. For more, please see pull request #918 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug there is a bug in the way jimp behaves
Projects
None yet
Development

No branches or pull requests

4 participants