Update to major version 5

mxmason commented 3 years ago

Summary

Decouplesnode-sass from this module, and requires users to explicitly set a compiler of their choice.
Upgrades our out-of-date dependencies (which also requires some minor changes to the test suite).
Bumps minimum Node version to 12.

Now that we require users to install their own compilers, it is not strictly necessary for us to bump our minimum Node version, but supporting Node versions that have hit EOL could complicate things in the future. It's worth noting that node-sass has adopted Node 12 as a minimum requirement.

New API

Major version 5 requires users to explicitly set a compiler by passing it to a curried function (per https://github.com/dlmanning/gulp-sass/pull/802#issuecomment-861236154). Both node-sass and sass compilers are permitted.

For example, in gulpfile.js:

# v4
- const sass = require('gulp-sass');
- const dartSass = require('sass');
- sass.compiler = dartSass;
# v5
+ const sass = require('gulp-sass')(require('sass'));

If the compiler is not set, gulp-sass will throw an error advising the user of this API.

Miscellaneous fixes

These issues relate either to the current version of the project bundling node-sass@4, or to our dependencies currently being outdated.

Issue list

- resolves #801 - resolves #797 - resolves #785 - resolves #782 - resolves #780 - resolves #799 - resolves #798 - resolves #775 - resolves #768

Tasks

[x] Move node-sass to dev dependencies; update to version 6
[x] Update gulp to major version 4, to support new node-sass
- [x] Update gulp plugins used for tests
- [x] Update mocha tests for gulp compatiblity
[x] Change API for setting compiler, update tests to match
[x] Add new error about setting compiler
[x] Update other dependencies not broken by node-sass / Node 12 for better future stability
[x] Update readme, changelog
- [x] Add a warning that fibers is not supported in Node 16 (per #771). Best practice is to use sync importers for now, and the Dart Sass team is working on a fix.
[ ] Update tests to provide confidence about dart-sass as compiler See https://github.com/dlmanning/gulp-sass/pull/802#discussion_r652940083

xzyfer commented 3 years ago

Thanks for all this effort. The comment about broad version support was with intention of not needing to bump the major to limit the impact on the ecosystem.

I agree the changes to support versions in node-sass forces our hand here, and we had planned to bump the major in order to make node-sass optional anyway.

xzyfer commented 3 years ago

Can the previously proposed updates the CONTRIBUTING.md and README.md also be ported into this PR?

xzyfer commented 3 years ago

Additionally, given we're bumping the major and updating the gulp dependencies, are you to confirm whether these proposed README.md make sense to be ported to this PR also?

Apologies I'm not in touch with gulp anymore.

mxmason commented 3 years ago

Thanks for looking at my work! I can give the changes to Readme and Contributing a more thoughtful look tomorrow!

I noticed that the initial API for setting the compiler had users passing an argument, as with gulpSass(compiler). Should I revert to that, or keep the API where users set a compiler prop after defining gulpSass?

xzyfer commented 3 years ago

When I used gulp heavily the gulpSass(compiler) was a common pattern so I'd favour that approach unless there's a strong argument against it. I believe that linked commit is fairly close to feature complete, so I'd suggest taking that a starting point for this work.

xzyfer commented 3 years ago

That's for all the great effort. I'll take a closer look in the next couple days.

mxmason commented 3 years ago

Thanks, @xzyfer! I still need to update the docs, and I can do that over the next couple days if you want!

xzyfer commented 3 years ago

Awesome!

mxmason commented 3 years ago

Hey, @xzyfer I've updated the README file to

document the new API
provide a short migration guide
warn about the EOL of node-fibers (we now suggest switching to renderSync)
use examples written for Gulp 4
include new badges to provide more info about this package

Here's the rendered markdown.

I have not updated the Changelog yet, as I saw that the Changelog consists mostly of your cut releases, and i can't cut those. Let me know if I should do anything else!

Edit: Don't know why I tagged Dylan Manning in this comment at first; apologies!

xzyfer commented 3 years ago

Amazing work!

cyrilverloop commented 3 years ago

Hi,

Thanks for this update. Is it possible to import this as an ES module ? Like : import sass from 'gulp-sass';

mxmason commented 3 years ago

Hey @cyrilverloop,

This release can’t be imported as an esmodule. I’m open to working on a release that supports both commonjs and esmodules (if @xzyfer thinks it’d be useful), but that would take a bit of effort.

xzyfer commented 3 years ago

Would this also require dart-sass and node-sass adopt esm?

On Sun, 27 Jun 2021, 9:22 am EJ Mason, @.***> wrote:

Hey @cyrilverloop https://github.com/cyrilverloop,

This release can’t be imported as an esmodule. I’m open to working on a release that supports both commonjs and esmodules (if @xzyfer https://github.com/xzyfer thinks it’d be useful), but that would take a bit of effort.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dlmanning/gulp-sass/pull/802#issuecomment-869073122, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAENSWB7GN62JFOURM64FCDTUZOKLANCNFSM46WECMZQ .

cyrilverloop commented 3 years ago

I don't know about dart-sass and node-sass, but using import worked in gulp-sass v4.

cyrilverloop commented 3 years ago

Can the compiler be defined after the import ?

mxmason commented 3 years ago

I might be wrong about ESM support here! I’ll test it when I’m at my desk. But yes, the compiler can be defined after import.

mae829 commented 3 years ago

@cyrilverloop not sure you figured it out but this is my implementation after updating to v5

import dartSass from 'sass';
import gulpSass from 'gulp-sass';
const sass = gulpSass( dartSass );

cyrilverloop commented 3 years ago

@mae829 thanks for the import. Your solution works for me.

I recommend to put it in the documentation.

mxmason commented 3 years ago

Thank you, @mae829! I agree that this should be in the docs and I will make a PR about it soon.

dlmanning / gulp-sass