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

npm package for browser #22

Closed
cburgmer opened this issue Nov 25, 2013 · 6 comments
Closed

npm package for browser #22

cburgmer opened this issue Nov 25, 2013 · 6 comments

Comments

@cburgmer
Copy link
Contributor

I am in the process of moving all dependencies from bower to npm (a good read is http://dontkry.com/posts/code/browserify-and-the-universal-module-definition.html). Now I would like to depend on imagediff through npm for browser-facing code.

The npm package for imagediff however depends on canvas which itself has a dependency (cairo) that needs manual installation. For browser-facing code this dependency is not needed and this manual installation step is a deal breaker.

It would be super helpful if we could provide a npm package without that strictly required dependency. I see two options here:

  1. Publish a second package to npm just for the use in browsers, say imagediff-browser.
  2. Make the dependency on canvas optional (via optionalDependencies, https://npmjs.org/doc/json.html#optionalDependencies).

Any more that I missed?

Personally I would prefer option 2. This would be as simple as changing "dependencies" in package.json to "optionalDependencies". npm will still try to install canvas, but will skip if installation fails and return successfully.

The issue with step 2 is that NodeJS users will yield a working dependency installation while the setup is not complete. One way of mitigating that would be to throw an error indicating the unmet dependency.

What do you think?

@cesutherland
Copy link
Member

I'd love to support browserify, but I'd hope to do it transparently.

I'll need to read a bit, but maybe there's another option. What do you think of this? https://npmjs.org/package/canvas-browserify

@cburgmer
Copy link
Contributor Author

I still need to understand how canvas-browserify transparently switches between node Canvas and HTML5 CanvasElement but as far as I understand in the end this is syntactic sugar (sugar is good though :). If you look at dominictarr/canvas-browserify#2 it seems to have the same problems I have here.

@cburgmer
Copy link
Contributor Author

Looking at the commit now it looks like canvas-browserify solves the problem using option 2.

@cesutherland
Copy link
Member

If I understand the abstraction correctly, turns out this isn't for solving a dependency issue it just replaces the need for https://github.com/HumbleSoftware/js-imagediff/blob/master/js/imagediff.js#L28 with the sugar.

So we can implement option 2, document it, and supply a specific error message to the user.

Ultimately, it is probably best to submit a PR to node-canvas that fixes it for browserify.

@cburgmer
Copy link
Contributor Author

cburgmer commented Mar 5, 2014

I am unsure what can be changed in node-canvas to make it work for browserify. I submitted a PR so that we can do the part on our side here.

@cesutherland
Copy link
Member

I've added some documentation and merged the PR.

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

No branches or pull requests

2 participants