search

sass / node-sass

:rainbow: Node.js bindings to libsass
https://npmjs.org/package/node-sass
MIT License
8.51k stars 1.33k forks source link

Consider splitting out the CLI as a separate module #1156

Open callumlocke opened 9 years ago

callumlocke commented 9 years ago

After reading sindresorhus's reasons for splitting out the CLIs from his modules, and seeing that Babel does a variation of this too (providing a CLI-free 'core' module), I've come to think it's a better approach.

I suggest making a new module, node-sass-cli, which would be a simple CLI wrapper for node-sass. (And remove all the CLI-related deps and code from node-sass itself.)

The benefits are:

  1. Quicker and less error-prone to install node-sass as a dep. Currently, if I want to depend on node-sass, I am forced to download and install a redundant file watcher too, which itself has a bunch of dependencies plus a postinstall script that takes minutes and can even error out sometimes.
  2. Separation of concerns. The -cli module can just focus on being a good CLI.
  3. One is a global application, the other is a library for importing into your code. Separating them would allow for shorter and more focused readmes.

Examples:

saper commented 9 years ago

We still find some bugs in the "lib" code with cli.js, I'd love to have the parity between api.js tests and cli.js.

lib/render.js seems to live kind of on its own (used only by the CLI), good question where it should land, should we split.

And I think that bin/node-sass has a lot of redundant code and can be actually made much smaller.

I think in close future we will change the interface between the C++ bindings and the Javascript code and simplify JavaScript side a lot. Since I have introduced test/lowlevel.js to see how a "pure" C++ API feels like. Lots of similarities with what the CLI is doing actually...

But to me a priority would be to de-orbit sass_types and write them in JavaScript (#1088). For example, we could add some of the existing color modules as a dependency and just ask bindings to create /accept instances of it. And finally use normal null and JavaScript strings. We could have sass-map, sass-list modules if needed. (Unfortunately we cannot rely on tuples here).

sindresorhus commented 9 years ago

:+1:

thusoy commented 7 years ago

@saper From what I can tell, this would cut the install size for the core with 14MB (on disk, from 26MB to 12MB) by getting rid of request, meow, gaze, npmlog, async-foreach, cross-spawn, get-stdin, in-publish (should be devdependency?) and sass-graph, which are only used for the cli. Not to mention that the shrinkwrap is cut from the current 824 lines down to 191, and the corresponding less requests done to npm to fetch all of these things.

saper commented 7 years ago

Looking at some open CLI issues I am getting more and more convinced this is the way to go. I'd love to fix the C binding API to make it usable without excessive wrapping as we do currently plus moving watching and other stuff into a separate possibly advanced command-line module.

xzyfer commented 7 years ago

Let's see how we feel post v5. There'll be significant clean ups in this area that may make this less relevant.

On 13 Nov. 2017 8:44 am, "Marcin Cieślak" notifications@github.com wrote:

Looking at some open CLI issues https://github.com/sass/node-sass/issues?q=is%3Aissue+is%3Aopen+label%3A%22Module+-+CLI%22 I am getting more and more convinced this is the way to go. I'd love to fix the C binding API to make it usable without excessive wrapping as we do currently plus moving watching and other stuff into a separate possibly advanced command-line module.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/sass/node-sass/issues/1156#issuecomment-343770538, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjZWGETSZyl2pO1vr32HYLn3c9mXD31ks5s12bQgaJpZM4F_eIt .