I'd have thought most developers will now be using node 12 or later on their environments. If this is used alongside gulp 3 or less a primordials error will be seen when running gulp commands.

It looks like this projects gulp tasks will need a minimal amount of updates to get it working with gulp v4.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alanoakden created an issue. See original summary.

alanoakden’s picture

alanoakden’s picture

Status: Active » Needs review
sasanikolic’s picture

Status: Needs review » Needs work

Hi @alanoakden, thanks for your contribution.

I tested your changes with node v10 and v12, but a couple of things doesn't seem to work correctly yet:

  • The sass linter doesn't seem to work anymore. Before, you had stylelint warnings displayed in the console when running gulp.
  • I tried making a change in a sass file, but the expected css didn't get compiled.
alanoakden’s picture

Interesting, which SASS file didn't compile?

I've basically just captured the changes i made to even get everything working in the first place on my local.

sasanikolic’s picture

I tested with paragraphs.widget.scss and paragraphs.modal.scss.

Node versions v10.21.0 and v12.18.2.

sasanikolic’s picture

Did some quick debugging and found out that options.scssSrc is defined as "./", therefore the paths were wrong and sass and linter didn't work correctly, as the path was then ".//*.scss". Not sure how this worked before... Fixed this in this patch.

pivica’s picture

Tested fully latest patch including CSS compilation, works as expected and do not produce any differences in CSS.

Patch is huge because we are adding package-lock.json but it makes sense to add this so everybody are using same libraries and results are consistent.

> Not sure how this worked before...

Probably previous versions of gulp handled that differently and with latest updates this now throws error which is a correct behaviour because it is a wrong path.

Additionally changed documentation about running gulp with `npx gulp`, I guess it makes more sense because you don't need to have gulp installed globally and npx is now shipped with npm.

sasanikolic’s picture

Got a bit carried away when reviewing the readme file, with so many grammar mistakes... :)

Let me know if you want me to create a followup for those fixes or if doing that in this patch is fine.

Berdir’s picture

Status: Reviewed & tested by the community » Fixed

Ha. Fixed typos, you mean added 10 "the"'s ;) (Yes, I counted them).

Fine with me, committed.

  • Berdir committed 2223a6f on 8.x-1.x authored by sasanikolic
    Issue #3155396 by sasanikolic, pivica, alanoakden: Upgrade Gulp to...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.