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

Can the node-canvas dependency be made a peer dependency? #1252

Closed
kumarharsh opened this issue Feb 6, 2017 · 39 comments
Closed

Can the node-canvas dependency be made a peer dependency? #1252

kumarharsh opened this issue Feb 6, 2017 · 39 comments
Assignees

Comments

@kumarharsh
Copy link

kumarharsh commented Feb 6, 2017

On one of my projects using paperjs, every single npm install node tries to install the canvas dependency. This fails (because I don't have cairo, etc) and I since I'm going to use this purely in browser, I will never need node-canvas to be installed. But the way npm install works, since it doesn't find canvas already in the node_modules folder, it tries to install canvas on each try, thus slowing down every single instance of npm install. I believe moving canvas to a peer dependency would solve this issue. Also, it will mean that the developer has to explicitly specify that their project depends on node-canvas, which I find to be more desirable. What do you think?

@lehni
Copy link
Member

lehni commented Feb 6, 2017

What's a peer dependency? I never heard of the concept. I agree that with more and more people using NPM for browser libraries, we have to find a better solution.

@lehni
Copy link
Member

lehni commented Feb 6, 2017

Related: #1171 (comment)

@kumarharsh
Copy link
Author

I think this article may explain it better: https://nodejs.org/en/blog/npm/peer-dependencies/

@lehni
Copy link
Member

lehni commented Feb 7, 2017

Hmm but this doesn't sound like it would solve the issue. Peer dependencies will have to be installed, no?

@kumarharsh
Copy link
Author

Yes, after giving it a little thought I see that it might not be the best way. Let me try figuring out something which solves this issue. I'll ping back with some results soon.

@lehni
Copy link
Member

lehni commented Feb 8, 2017

We already define it as an optional dependency, but these get installed by default, unless you choose to exclude it:

npm install paper --no-optional

@lehni
Copy link
Member

lehni commented Feb 8, 2017

Here's another idea:

We could have a pure paper module with no dependencies, a paper-headless that only requires paper and jsdom, and a paper-canvas that requires paper, canvas and jsdom. This would allow the maintaining of different, clearly separated targets. It's a shame that NPM does not allow the definition of such targets, with separate dependencies... I could include the process of keeping the versions up to date and in sync as part of the publishing gulp task...

@kumarharsh
Copy link
Author

I think there are plugins which might help with this such as Lerna: https://github.com/lerna/lerna

@lehni
Copy link
Member

lehni commented Mar 10, 2017

@kumarharsh thanks for the hint, that looks interesting. I shall investigate this further.

lehni added a commit that referenced this issue Mar 19, 2017
lehni added a commit that referenced this issue Mar 20, 2017
Planend are the modules paper-jsdom and paper-jsdom-canvas, as shim modules that require and handle the dependencies as peer dependencies.

Relates to #1252
@lehni
Copy link
Member

lehni commented Mar 20, 2017

I have started implementing this now in 29de03d:

Planend are the modules paper-jsdom and paper-jsdom-canvas, as shim modules that require and handle the dependencies as peer dependencies. It works well in my local tests already.

The core paper module will then have no dependencies itself, but is attempting to load these libraries and fail silently. If the library is loaded through one of the sims, then an exception is thrown if the dependency is missing.

@simonemazzoni
Copy link

@lehni any news about this? I see it hasn't been merged into master yet.

@lehni
Copy link
Member

lehni commented Apr 10, 2017

I haven't found time yet to work on this...

@simonemazzoni
Copy link

@lehni ok, I understand.
Could you provide an estimate time? Also, I don't know exactly what you have in mind to "split" the two parts, but if I can help in some way, let me know.

@kumarharsh
Copy link
Author

+1. @lehni I'd also like to lend my hand, if you can tell me what to do.

@lehni
Copy link
Member

lehni commented Apr 11, 2017

@tom10271 has pointed out a way to achieve this now already, here:

#1308 (comment)

Unfortunately I can't provide reliable estimates. I take care of paper.js in my spare time, and am not in a position to predict how much time I can devote to it at the moment...

@kumarharsh the plan is outlined above, but what I haven't figured out is how to handle versions... Will paper-jsdom and paper-jsdom-canvas be version locked to paper? If so, then we need an automated script that produces these and publishes new versions. That's the main missing part right now.

But maybe, the trick found by @tom10271 is good enough to not have to take care of such separate packages?

@simonemazzoni
Copy link

@lehni Thanks for the suggestion. That workaround is the one I am using now to workaround the problem.

Regarding the split I think you're right, the version should be locked to paper.
My question is, how is generated the paper package for bower? I noticed that it doesn't install the canvas module, but copies only the paper-core, paper-full files.

@lehni
Copy link
Member

lehni commented Apr 11, 2017

Bower is for web projects only, the node-canvas module isn't even in its registry : )

@simonemazzoni
Copy link

simonemazzoni commented Apr 11, 2017

@lehni yes I know. Sorry, bad choice of words.
I just self answered my real question. :)

Btw, #1308 (comment) doesn't seem to work. I'm investigating to check if I made something wrong in my package.json definition.

@kumarharsh
Copy link
Author

@lehni I don't think the format @tom10271 suggests in the linked comment is recognized by npm. Ref: https://docs.npmjs.com/files/package.json#dependencies
Also, when running yarn install, an error is thrown, which points to the same thing:

image

@kumarharsh
Copy link
Author

@lehni can you push the changes you've done (as described here) into a new branch, maybe I can take a shot at completing it?

@tom10271
Copy link

I have an updated solution. Please check: #1308 (comment)

Thanks

@simonemazzoni
Copy link

Tested right now! It works like a charm.
Good catch @tom10271

@kumarharsh
Copy link
Author

It works. For yarn, you'd need to run yarn install --ignore-scripts. But these are stop-gap solutions, and won't really work in the long run. For example, what if you need another npm package which has optional deps you need? I could go on with these scenarios (have run into a few myself), but I think you get the idea. If I'm lucky, I'll take a shot at untangling paperjs this weekend. Lets see how it goes :)

@lehni
Copy link
Member

lehni commented Apr 12, 2017

Yeah we need a proper fix. @kumarharsh I had it working locally, the missing bit is only the automatic publishing of these shim modules with each publishing of paper itself... This needs a gulp task.

@lehni
Copy link
Member

lehni commented Apr 12, 2017

e.g.:

paper-jsdom/index.js:

// Just pass through. The rest is handled by the paper.js library itself:
module.exports = require('paper');

paper-jsdom/package.json:

{
  "name": "paper-jsdom",
  "version": "0.10.4",
  "description": "The Swiss Army Knife of Vector Graphics Scripting, packaged for headless use in Node.js",
  "license": "MIT",
  "homepage": "http://paperjs.org",
  "repository": {
    "type": "git",
    "url": "git://github.com/paperjs/paper-jsdom"
  },
  "bugs": "https://github.com/paperjs/paper.js/issues",
  "author": "Jürg Lehni <juerg@scratchdisk.com> (http://scratchdisk.com)",
  "main": "index.js",
  "files": [
    "index.js",
    "README.md"
  ],
  "engines": {
    "node": ">=4.0.0"
  },
  "dependencies": {
    "paper": "0.10.4",
  },
  "peerDependencies": {
    "jsdom": "^9.4.0",
    "source-map-support": "^0.4.0"
  },
  "browser": {
    "jsdom": false,
    "jsdom/lib/jsdom/living/generated/utils": false,
    "source-map-support": false
  },
  "keywords": [
    "vector",
    "graphic",
    "graphics",
    "2d",
    "geometry",
    "bezier",
    "curve",
    "curves",
    "path",
    "paths",
    "canvas",
    "svg",
    "paper",
    "paper.js",
    "paperjs"
  ]
}

@lehni
Copy link
Member

lehni commented Apr 12, 2017

The part I wasn't so sure about was "peerDependencies" vs "dependencies".

@kumarharsh
Copy link
Author

IMO, you can put jsdom in the dependencies list for paper-jsdom, as the intent of the user is clear that they want jsdom to be installed.

@lehni
Copy link
Member

lehni commented Apr 12, 2017

Yeah I agree...

@lehni
Copy link
Member

lehni commented Apr 19, 2017

I am on this now, expect v0.10.4 shortly with these new modules.

lehni added a commit that referenced this issue Apr 19, 2017
@lehni
Copy link
Member

lehni commented Apr 19, 2017

Alright, it's all published now!

https://www.npmjs.com/package/paper
https://www.npmjs.com/package/paper-jsdom
https://www.npmjs.com/package/paper-jsdom-canvas

Please test and let me know if it works.

@lehni lehni closed this as completed Apr 19, 2017
lehni added a commit that referenced this issue Apr 19, 2017
@lehni lehni reopened this Apr 19, 2017
@lehni
Copy link
Member

lehni commented Apr 19, 2017

Oh darn, this should really be v0.11.0, not v0.10.4, because it will break existing Node.js projects that require paper with canvas support. I unpublished v0.10.4 and will publish v0.11.0 in a bit.

@lehni
Copy link
Member

lehni commented Apr 19, 2017

Alright, v0.11.0 is out now, and the automatic updating of the sub-modules (NPM shim packages) has also been tested now: https://github.com/paperjs/paper.js/blob/develop/gulp/tasks/publish.js#L57

@lehni lehni closed this as completed Apr 19, 2017
@simonemazzoni
Copy link

Tested it, and it looks good to me.

@kumarharsh
Copy link
Author

Perfect! 🎉

@lehni
Copy link
Member

lehni commented Apr 21, 2017

Great to hear! I was thinking that node folks could also do this instead, in the main project:

  "dependencies": {
    "canvas": "^1.3.5",
    "jsdom": "^9.4.0",
    "paper": "^0.11.2",
    "source-map-support": "^0.4.0"
  },

And after that, this would work just as well:

require('paper');

But the shim packages are still useful for easer dependency management and updating...

@kumarharsh
Copy link
Author

I'm using paper-jsdom for all the reasons you underlined above, as well as because when the next guy comes in and looks at the code, he'd be hard-pressed to figure out where jsdom is being used in the project.

@lehni
Copy link
Member

lehni commented Apr 21, 2017

Yes it does make sense. Glad to hear it works for you, too! : ) And updating happens automatically now through a Gulp tasks that keeps the versions in sync and publishes along with the main library, so it's not an additional managing effort.

@lehni
Copy link
Member

lehni commented Apr 21, 2017

While I was at it, I also streamlined the whole publishing process, which is now fully automated through Gulp. This also handles changes to the http://paperjs.org/ website, to which it now automatically pushes the new references, ZIP package and JS asset. And https://github.com/paperjs/paperjs.org now has a Travis CI worker that deploys the site statically to Github Pages at https://github.com/paperjs/paperjs.github.io every time a push happens to it. So releasing a new version unravels a sequence that results in everything being up to date.

Pure magic!

@kumarharsh
Copy link
Author

Sweet

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

No branches or pull requests

4 participants