search

twolfson / spritesmith

Utility that takes sprites and converts them into a stylesheet and its coordinates
MIT License
916 stars 56 forks source link

Defining engines as npm optionalDependencies leads to slower installation #38

Closed michaek closed 9 years ago

michaek commented 10 years ago

I don't think it's proper to define engine dependencies as optionalDependencies, because all engine dependencies (and their dependencies) are downloaded and compiled. It seems like the use of an optional dependency is more useful for leaving out features if you can't get the dependency or for providing a fallback.

Loading redundant modules when you only need one (or none) of them seems like a misuse of optionalDependencies. If there were no time penalty for installing the dependencies, this would be philosophical, but phantomjs takes a relatively long time to install, which is a shame if, for instance, you've set configuration to use graphicsmagick and your phantomjs goes unused.

Rather than having engine be a string that instructs spritesmith how to require the proper engine from its own dependencies, engine should accept the actual engine, such as require('phantomjssmith'). That makes the dependency of the host module more clear, as well as more efficient, and it simplifies spritesmith.

I have manually tested the assumption that this will work by removing engine detection code, and passing the engine in from the host module, but I have not rewritten any unit tests. I'll be happy to take that on if we agree on the direction.

twolfson commented 10 years ago

I agree with this sentiment. It was reasonable in the beginning when we had 2 engines (and needed fallbacks) but now with 5 (and 1 that is pure JS), we can probably move to the opt-in model.

If we do this change, then it should be a major release.

Additionally, it will relieve some of the complaints around npm install containing errors for those who don't have canvas/gm installed.

leevigraham commented 10 years ago

:+1:

Additionally, it will relieve some of the complaints around npm install containing errors for those who don't have canvas/gm installed.

This was exactly the first issue I encountered.

michaek commented 10 years ago

My changes are in forks to spritesmith and grunt-spritesmith. I'll get the tests passing and make a pull request.

twolfson commented 9 years ago

Taking this in another direction. I have finished a node-only spritesmith engine which will be the only default installed. All others will need to be explicitly specified/installed. I will comment on this issue when the new version is released.

twolfson commented 9 years ago

This has been completed in 1.0.0. There will be a grunt-spritesmith and gulp.spritesmith update later (documentation updates will be the limiting factor).