Make this package implementation-agnostic #694

nex3 · 2018-05-24T21:13:12Z

Rather than always loading Node Sass, this now requires users to pass
in either Dart Sass or Node Sass to the require() call.

Closes #672

Rather than always loading Node Sass, this now requires users to pass in either Dart Sass or Node Sass to the require() call. Closes dlmanning#672

nex3 · 2018-05-24T23:18:39Z

The linter wants me to add a trailing comma to my argument list, but it doesn't look like that works on Node 6.x. What should I do?

nex3 · 2018-06-04T17:38:04Z

@dlmanning Ping!

nex3 · 2018-06-13T19:58:48Z

@xzyfer As we discussed offline, I updated this to continue loading Node Sass by default, but to document support for Dart Sass and encourage users to explicitly choose their implementation. Please take a look.

This documents the option to use Dart Sass, and encourages users to explicitly choose their implementation, but it doesn't change the existing behavior.

MichaelFBA · 2018-07-24T07:26:42Z

Any chance we can get this merged? :)

devversion · 2018-07-25T21:34:17Z

Normally I'm not a fan of commenting in order to notify the project leads, but there hasn't been really much activity here. Please can someone have a look and give an update.

nschonni

Nothing blocking, just some feedback

nschonni · 2018-07-26T01:25:30Z

README.md

@@ -26,6 +26,8 @@ Something like this will compile your Sass files:
 var gulp = require('gulp');
 var sass = require('gulp-sass');

+sass.compiler = require(node-sass');


If node-sass is the default, this doesn't seem needed

nschonni · 2018-07-26T01:26:05Z

README.md

@@ -13,7 +13,7 @@ Only [Active LTS and Current releases][1] are supported.
 # Install

 ```
-npm install gulp-sass --save-dev
+npm install node-sass gulp-sass --save-dev


If it is still the included default this doesn't need to change

nschonni · 2018-07-26T01:28:49Z

README.md

+
+Note that when using Dart Sass, **synchronous compilation is twice as fast as asynchronous compilation** by default, due to the overhead of asynchronous callbacks. To avoid this overhead, you can use the [`fibers`](https://www.npmjs.com/package/fibers) package to call asynchronous importers from the synchronous code path. To enable this, pass the `Fiber` class to the `fiber` option:
+
+```javascript


Maybe mention the npm install dart-sass here?

nschonni · 2018-07-26T01:29:52Z

README.md

@@ -56,9 +60,36 @@ gulp.task('sass:watch', function () {
 });
 ```

+You can choose whether to use [Dart Sass][] or [Node Sass][] by setting the `sass.compiler` property. Node Sass will be used by default, but it's strongly recommended that you set it explicitly for forwards-compatibility in case the default ever changes.


The explicit compiler would be a breaking change and probably dealt with then instead of recommending it now. I'm just thinking if the future change happens it might be through a different method and this locks in the API when it's not needed yet

One way or another, there needs to be a way of overriding the compiler. That's the whole point of this change.

Technically overriding the compiler is already possible. Who knows what folks are doing with it. I'm OK with this because now we're at least documenting it as part of the API so we're committed to version correctly if we change.

nschonni · 2018-07-26T01:38:42Z

README.md

 ## Options

-Pass in options just like you would for [`node-sass`](https://github.com/sass/node-sass#options); they will be passed along just as if you were using `node-sass`. Except for the `data` option which is used by gulp-sass internally. Using the `file` option is also unsupported and results in undefined behaviour that may change without notice.
+Pass in options just like you would for [Node Sass](https://github.com/sass/node-sass#options); they will be passed along just as if you were using Node Sass. Except for the `data` option which is used by gulp-sass internally. Using the `file` option is also unsupported and results in undefined behaviour that may change without notice.


Should some validation around the file option be added, or is the dart-sass error user friendly?

Nothing about this behavior is changing. I just slightly reformatted the paragraph to refer to Node Sass more consistently.

nschonni · 2018-07-26T01:50:32Z

PS: I'm good if the future version tries to load sass first, and falls back to node-sass if the require fails (or add a flag to force node-sass). I'm thinking about the current changes through a "convention over configuration" lens.

nex3

My goal is to have the README encourage users to explicitly select the version of Sass they're using, so that in the future if the default changes or if an explicit decision is required--which is the way most plugins seem to be going--as many users' configurations as possible are already in a state that explicitly declares the Sass implementation they're using.

nex3 · 2018-07-26T23:00:12Z

README.md

@@ -56,9 +60,36 @@ gulp.task('sass:watch', function () {
 });
 ```

+You can choose whether to use [Dart Sass][] or [Node Sass][] by setting the `sass.compiler` property. Node Sass will be used by default, but it's strongly recommended that you set it explicitly for forwards-compatibility in case the default ever changes.


One way or another, there needs to be a way of overriding the compiler. That's the whole point of this change.

nex3 · 2018-07-26T23:00:43Z

README.md

 ## Options

-Pass in options just like you would for [`node-sass`](https://github.com/sass/node-sass#options); they will be passed along just as if you were using `node-sass`. Except for the `data` option which is used by gulp-sass internally. Using the `file` option is also unsupported and results in undefined behaviour that may change without notice.
+Pass in options just like you would for [Node Sass](https://github.com/sass/node-sass#options); they will be passed along just as if you were using Node Sass. Except for the `data` option which is used by gulp-sass internally. Using the `file` option is also unsupported and results in undefined behaviour that may change without notice.


Nothing about this behavior is changing. I just slightly reformatted the paragraph to refer to Node Sass more consistently.

nex3 · 2018-10-03T18:23:52Z

Ping!

xzyfer · 2018-10-16T07:57:42Z

Sorry for the delay. I took a break from GitHub.

xzyfer · 2018-10-16T08:03:25Z

Released as v4.0.2

brookback · 2018-12-04T21:47:25Z

Thanks for this!

But, would it not make sense to completely remove the dependency of node-sass, in order to avoid having binary dependencies? And to state in the README (as it is now) that people have to do

npm install --save gulp-sass node-sass

to get started?

tinyfly · 2019-01-31T19:42:12Z

But, would it not make sense to completely remove the dependency of node-sass

I would like this tremendously. I'm sure there are reasons for this but one of the big advantages for using dart-sass is not having to build node-sass and the complications that can come from that. As it is I'm still having to build node sass on install event if I choose not to use it.

xzyfer · 2019-01-31T21:15:15Z

You're welcome to fork this project or use gulp-dart-sass, but there are no plans change node-sass as the default.

* Update node-sass to 6.0.0 Also moves node-sass to devDependencies and peerDependencies. This helps prevent dupes and conflicts in packages using gulp-sass * Make mocha test compatible with gulp@4 * Update package version, engines * Remove node-sass as peer dependency * Replace through2 with transfob * Uninstall through2 * Throw error if compiler not set * Fix casing of Sass Co-authored-by: Michael Mifsud <xzyfer@gmail.com> * Use function arg to set compiler, use transfob package * Update test to new API signature * Update package versions * Fix eslint errors * Incorporate previously proposed documentation changes Copied from #694 * Update document outline, apply copyedits * Copyedit: overall clarifications * Call out migration guide, update badges * Document Gulp 4 usage * smol clarification * Copyedit some headings * Yet Another Copyedit™ * Update README.md Co-authored-by: Michael Mifsud <xzyfer@gmail.com>

Make this package implementation-agnostic

ffd3300

Rather than always loading Node Sass, this now requires users to pass in either Dart Sass or Node Sass to the require() call. Closes dlmanning#672

nex3 mentioned this pull request May 24, 2018

Add support for Dart Sass #672

Closed

Add a note about synchronous versus asynchronous speed

54641f5

nex3 mentioned this pull request May 28, 2018

Help migrate Node Sass tools to be implementation-agnostic sass/dart-sass#267

Closed

7 tasks

Continue loading Node Sass by default

4b673f6

This documents the option to use Dart Sass, and encourages users to explicitly choose their implementation, but it doesn't change the existing behavior.

nex3 force-pushed the dart-sass branch from 972a6d2 to 4b673f6 Compare June 13, 2018 20:39

nschonni reviewed Jul 26, 2018

View reviewed changes

Fix README typos

0ab5133

nex3 commented Jul 26, 2018

View reviewed changes

xzyfer merged commit 98254d2 into dlmanning:master Oct 16, 2018

stof mentioned this pull request Apr 12, 2019

Using dart sass still requires installing node-sass and its native addon #731

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make this package implementation-agnostic #694

Make this package implementation-agnostic #694

nex3 commented May 24, 2018

nex3 commented May 24, 2018

nex3 commented Jun 4, 2018

nex3 commented Jun 13, 2018

MichaelFBA commented Jul 24, 2018

devversion commented Jul 25, 2018

nschonni left a comment

nschonni Jul 26, 2018

nschonni Jul 26, 2018

nschonni Jul 26, 2018

nschonni Jul 26, 2018

nex3 Jul 26, 2018

xzyfer Oct 16, 2018

nschonni Jul 26, 2018

nex3 Jul 26, 2018

nschonni commented Jul 26, 2018

nex3 left a comment

nex3 Jul 26, 2018

nex3 Jul 26, 2018

nex3 commented Oct 3, 2018

xzyfer commented Oct 16, 2018 •

edited

Loading

xzyfer commented Oct 16, 2018

brookback commented Dec 4, 2018

tinyfly commented Jan 31, 2019 •

edited

Loading

xzyfer commented Jan 31, 2019


		Note that when using Dart Sass, synchronous compilation is twice as fast as asynchronous compilation by default, due to the overhead of asynchronous callbacks. To avoid this overhead, you can use the [`fibers`](https://www.npmjs.com/package/fibers) package to call asynchronous importers from the synchronous code path. To enable this, pass the `Fiber` class to the `fiber` option:

		```javascript

Make this package implementation-agnostic #694

Make this package implementation-agnostic #694

Conversation

nex3 commented May 24, 2018

nex3 commented May 24, 2018

nex3 commented Jun 4, 2018

nex3 commented Jun 13, 2018

MichaelFBA commented Jul 24, 2018

devversion commented Jul 25, 2018

nschonni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nschonni commented Jul 26, 2018

nex3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nex3 commented Oct 3, 2018

xzyfer commented Oct 16, 2018 • edited Loading

xzyfer commented Oct 16, 2018

brookback commented Dec 4, 2018

tinyfly commented Jan 31, 2019 • edited Loading

xzyfer commented Jan 31, 2019

xzyfer commented Oct 16, 2018 •

edited

Loading

tinyfly commented Jan 31, 2019 •

edited

Loading