Skip to content
This repository has been archived by the owner on Sep 29, 2021. It is now read-only.

Commit

Permalink
fix(gulp): Old-street style pipe works well!
Browse files Browse the repository at this point in the history
  • Loading branch information
@outloudvi
outloudvi committed Apr 26, 2020
1 parent 9321c86 commit dde81ca
Showing 1 changed file with 18 additions and 18 deletions.
36 changes: 18 additions & 18 deletions gulpfile.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,28 +41,28 @@ function js() {
const babel = require("gulp-babel");
const uglify = require("gulp-uglify");

return pipeline(
gulp.src(JS_DIR, OPTS),
webpack({
mode,
devtool: "source-map",
output: {
filename: "script.js"
}
}),
prodOnly(sourcemaps.init({loadMaps: true})),
prodOnly(through.obj(function (file, enc, cb) {
return gulp.src(JS_DIR, OPTS)
.pipe(
webpack({
mode,
devtool: "source-map",
output: {
filename: "script.js",
},
})
)
.pipe(prodOnly(sourcemaps.init({loadMaps: true}))
.pipe(prodOnly(through.obj(function (file, enc, cb) {
// Filter out the sourcemaps since gulp-sourcemaps handles them
if (!file.path.endsWith(".map")) this.push(file);
cb();
})),
prodOnly(babel({
}))))
.pipe( prodOnly(babel({
presets: ["@babel/env"]
})),
prodOnly(uglify()),
prodOnly(sourcemaps.write(".")),
gulp.dest(DEST)
);
})))
.pipe(prodOnly(uglify()))
.pipe(prodOnly(sourcemaps.write(".")))
.pipe(gulp.dest(DEST));
}

function css() {
Expand Down

8 comments on commit dde81ca

@Pizzacus
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope I'm not being indiscreet by commenting on this. Of course, since it is your project, you should feel free to refactor the code in whichever way feels the most comfortable for you! (In fact, given the quality of the code on satania.moe, I would encourage you to)

However, the use of the pipeline function has several advantages. Beyond the fancy syntax, it normalises several aspects of streams in a chain like that.

  • It will destroy all steams once the final stream has ended, which is just good practice. Since the transform streams used by Gulp only modify the data they are given, they have no way of knowing when they should end and clean up, which is usually fine but still a waste of resources

  • It will listen for errors on all the streams and immediately reject as soon as an error is fired.

That last point is especially important. In old pipes, when a stream emits an error, it will not be passed down the chain. Making error handling impossible. Because of this, a stream chain with proper error handling would look like this

  return gulp.src('lib/*.js')
    .on('error', createErrorHandler('gulp.src'))
    .pipe(uglify())
    .on('error', createErrorHandler('uglify'))
    .pipe(gulp.dest('dist'))
    .on('error', createErrorHandler('gulp.dest'));

This snippet comes from this page of the Gulp GitHub docs (their GitHub docs are different from those of their website) in which they recommend to use the module pump, however, the native pipeline function since Node 10 (which is beyond LTS now) does the same as that module does (but takes multiple arguments instead of arrays of streams).

  pipeline(
    gulp.src('lib/*.js'),
    uglify(),
    gulp.src('lib/*.js')
  );

Pipeline also returns a Promise instead of a Stream. In the context of creating "tasks" that have a clear end, it can be easier to deal with Promises. Gulp can totally deal with promises instead of streams.

For instance, it is easy to use .then on the Promise, or use async functions, to run code that happens at the end of the task, and thus delay the end of it for Gulp. You could previously use the end event of a stream to run something after it, but Gulp wouldn't wait for it, which could cause problems with series and stuff like that.

async function css() {
  const files = [];

  await pipeline(
    gulp.src("src/css/*.css"),
    through.obj(file => {
      files.push(file.basename)
    }),
    compile(),
    minimize(),
    gulp.dest("dist/css")
  );

  // We can run extra code after this pipeline easily!
  // Here, we want to make a css file that will @import all the others
  // Generally, you would concat them instead, but here we'll assume you don't want to
  // Maybe to retain the ability to add files one by one if needed

  const content = files.map(
    file =>`@import '${file}';`
  ).join("\n");

  fs.writeFileSync("dist/main.css", content);
}

But unfortunately, the Gulp community is a bit hypocritical since they too have used old pipes in other parts of the docs. In my opinion, Gulp should recognise the importance of using pipelines and use them all the time, especially when this is now natively supported by Node.js.

People like the .pipe syntax, so unlike all the ES6 improvements that were adopted quickly, people don't see the benefit in using pipeline at first, so that's why it's important the Gulp community help explain it to people.

Maybe someday, we'll be able to make use of the proposed function pipeline operator but that would require rewriting a lot of Gulp plugins ^^

@outloudvi
Copy link
Author

@outloudvi outloudvi commented on dde81ca Apr 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, impressive explanation for me a Gulp green-hand! Thanks for your words @Pizzacus! ❤️
I'm really a Promise advocate since it simplifies the process of handling errors (ofc as well as many other reasons), and as you said, the feature is not present on old-style Gulp pipes. Given the benifites of the pipeline() I would also go for this. However, I'm not a pro user of Gulp (also not Grunt and not even Webpack), so this commit is just an attempt to make this project build successfully. The only reason I'm switching is that the original Pizzacus/satania.moe@3124cb12 didn't build on my computer, and the first solution to make it build was using the old-style pipes. I'll be more than happy to switch back if your project builds with the Gulpfile, no matter it's because of an upstream update of any update on you side.

This project is forked from your side, and you apparently know much more than Gulp than me, so I will always be with you here. Cheers :D

@Pizzacus
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's weird that pipeline doesn't work for you, are you on the latest Node LTS?

node -v

v12.16.1

Pipelines were introduced in v10, now the LTS is v12

And yeah, Gulp isn't really that hard to use, since fundamentally, all it does is run functions. The problem is that too many people who use Gulp still use deprecated patterns, or make things uselessly complicated by thinking that you should only ever use streams in Gulp and nothing else. In reality, Gulp did promote using streams for parallel building, but it was never the only thing you should use.

Like the gulp-prompt package. That's rediculous. That is logic, like if-else statements, that is not something streams were designed for. Don't use gulp-prompt, use a general prompt package, ask for stuff before running the task, and adapt it accordingly.

I am sure this is what makes people feel like Gulp is so confusing. They see everyone using streams in all these weird ways and think "why do it like that", and the answer is you shouldn't. Streams are confusing enough as they are, you don't need to inflict pain upon yourself by only using them in your tasks.

I think this is because Gulp v4, which made everything much more modern and nicer, took waaaaaay too long to be released, and because of that, nobody used its features. And as such, Gulp has acquired a bad reputation, and a lot of people see it as a thing of the past, I don't really understand why it would be though, but I understand why they feel this way. These mistakes are now too widespread and harmful... Although the official docs are okay (except that they don't use pipelines)

I like Gulp's transparent approach, where you set up a stream and you know how files get processed, and you can easily modify this process if you want. In fact, Gulp is so flexible it could absolutely be used to compile other languages than web projects. It's like a Makefile in js.

Whereas to me, Webpack, when you rely 100% on it as most projects do, is incredibly opaque. I am not sure if this is actually the case, or if it is just that I don't understand its inner-workings well enough. Although Gulp has no inner-workings like that, what you see is what you get. Gulp has this Unix philosophy where packages should only do a few things and do them well. Maybe that's why I use Webpack only for JS packaging in the middle of a Gulpfile, which a lot of Webpack users would disapprove of. I love the way Webpack packages JS so you can use dependencies like you do in Node on the browser. But managing the build process of a website is, I think, very different from packaging files. That's why I don't understand how people have come to rely on Webpack for the whole build process when it was made as a JS packager.

But then again, maybe I just don't understand Webpack enough to understand the benefits... I did try it, even thinking it would be better, maybe I didn't get far enough with that. Like these are just my impressions, but I'm not confident enough to say I don't recommend it. Surely it's gone so popular, so many people switched to it, there has to be a reason right?

@outloudvi
Copy link
Author

@outloudvi outloudvi commented on dde81ca Apr 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Pizzacus Oh, I can also build this over Node.js 12 (Arch Linux package community/nodejs-lts-erbium 12.16.2-1)! However it doesn't build with the latest Node.js version:

[18:44:06] 'js' errored after 1.96 s
[18:44:06] Error [ERR_STREAM_PREMATURE_CLOSE]: Premature close
    at Duplexify.onclose (internal/streams/end-of-stream.js:105:38)
    at Duplexify.emit (events.js:315:20)
    at Duplexify.EventEmitter.emit (domain.js:547:15)
    at Duplexify._destroy (/tmp/satania.moe/node_modules/ternary-stream/node_modules/duplexify/index.js:203:8)
    at /tmp/satania.moe/node_modules/ternary-stream/node_modules/duplexify/index.js:185:10
    at processTicksAndRejections (internal/process/task_queues.js:79:11)
[18:44:06] 'build' errored after 2.03 s
[18:44:06] The following tasks did not complete: assets
[18:44:06] Did you forget to signal async completion?
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

And yes, it's not Node.js 12. As of the version...

$ node -v
v14.0.0

P.S. Oh... I just found that the build error of Pizzacus/satania.moe seems to be a weird platform error of Zeit (now it's Vercel), not the code.

P.S.S. Now the only reason I'm keeping this legacy pipeline is for the build experience of some bleeding-edge Node.js users (like me and many other Arch Linux users of suisei-cn organization XD)

And finally the answer:

It's weird that pipeline doesn't work for you, are you on the latest Node LTS?

tl;dr no. (oh buddy the tl;dr shouldn't be here really)

@Pizzacus
Copy link

@Pizzacus Pizzacus commented on dde81ca Apr 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm the website doesn't build on Node v14, seems like the bug is due to the change in stream behaviour where streams now close automatically after they end... It seems the issue happens in webpack-stream or one of its dependencies, for now it should be best not to use Node v14, as it appears the version of Chokidar used, which is used to watch files, doesn't support it anymore either...

I've never seen a Node release break so many things wow x_x

Anyway, I won't let it hit LTS in October without satania.moe being compatible with it, but I believe it's a dependency issue, so we can only wait...

v14 only came out days ago so it's fine I guess...

I understand everyone have different standards for what versions of Node should be supported, but Node v8 has reached end-of-life, it will no longer be updated for critical changes, so I don't think anyone should be using it... Node v10 is the oldest supported version :x

Edit: But yeah for now we are stuck between one hack (using the LTS) or another (using .pipe) so I guess using pipes for now is fine, I think pipes fix it because of the fact that they handle errors badly. The webpack error probably doesn't actually matter since it is emitted after the stream is closed, and so since pipes discard it, the stream ends sucessfully.

@outloudvi
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you, especially for the edited appendix. "Premature close" doesn't seem to be a error of the code itself.
Finally, let's wait for the upstream fix for this, and cheer for it when the fix lands! 🎉

@Pizzacus
Copy link

@Pizzacus Pizzacus commented on dde81ca May 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: this appears to be an issue in Node.js itself nodejs/node#32968

Edit: Okay yep! If you upgrade to v14.1.0, pipeline now works fine!

@outloudvi
Copy link
Author

@outloudvi outloudvi commented on dde81ca May 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh, Pizzacus/satania.moe@3124cb1 didn't build on node v14.0.0, and now builds on node v14.1.0.
Thanks, Pizzacus :) I'll revert this on this repository.

Edit: it's done in 9b35ff5.

Please sign in to comment.