Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Monkey patching pipe method from gulp.src #91

Closed
floatdrop opened this issue Jan 5, 2014 · 6 comments
Closed

Monkey patching pipe method from gulp.src #91

floatdrop opened this issue Jan 5, 2014 · 6 comments

Comments

@floatdrop
Copy link
Contributor

In continue from this issue and summed up thoughts about error handling in gulp here is possible solution. Monkey patch pipe method from gulp.src in such a way, that it will monkey patch other streams, when you call pipe on them. This will allow fix pipe behaviour, that unpipes destanation stream on error.

Here is proposed options to gulp.src:

unpipeOnError [Boolean]
Don't remove ondata listener in destination stream on error. This option inherits on all streams in pipeline.

logOnError: [Boolean, String]
Little helper, that cleans up copy-pasted on('error', gutil.log) code. It will log errors that prefixed by passed string.

Example of usage:

gulp.src('coffee/**/*.coffee', { unpipeOnError: false, logOnError: true })
  .pipe(gulpPrefixer('// Copyright 2014 (C) Aswesome company'))
  .pipe(coffee())
  .pipe(gulp.dest('js/'));

Also it will be nice to have unpipeOnError option inside a pipe function to allow partially soft pipelines:

gulp.src('coffee/**/*.coffee', { logOnError: true })
  .pipe(gulpPrefixer('// Copyright 2014 (C) Aswesome company'))
  .pipe(coffee(), { unpipeOnError: false })
  .pipe(gulp.dest('js/'));
@yocontra
Copy link
Member

yocontra commented Jan 5, 2014

This seems overcomplicated...

@dashed
Copy link
Contributor

dashed commented Jan 5, 2014

I'm for logOnError: true or logOnError: gutil.log config option.

I'm little unclear on how unpipeOnError resolves the issues mentioned in your gist. I haven't looked extensively under the hood of node.js streams.

@floatdrop
Copy link
Contributor Author

This issue affecting gulp-watch as well, if error occured in plugin - this plugin will be unpiped from source and will not be receiving vinyl files.

@floatdrop
Copy link
Contributor Author

I published gulp-plumber which will contain some "pipe-helpers". For now it only fixes unpiping behaviour on all streams, that are down the pipeline.

var gulp = require('gulp'),
    watch = require('gulp-watch'),
    plumber = require('gulp-plumber'),
    sass = require('gulp-sass');

gulp.task('default', function () {
    gulp.src('scss/**', { read: false })
        .pipe(watch())
        .pipe(plumber())
        .pipe(sass())
        .pipe(gulp.dest('./dist/'));
});

@dashed
Copy link
Contributor

dashed commented Jan 6, 2014

gulp-plumber works pretty well. People using this, however, should be aware of plugins using es.map.

@tennessine
Copy link

thanks

wincent added a commit to liferay/liferay-js-themes-toolkit that referenced this issue Apr 8, 2019
When deploying a theme using this config in the package.json:

    "liferayTheme": {
            "baseTheme": "styled",
            "distName": "fjord-theme",
            "rubySass": false,
            "version": "7.1",
            "postcss": [
                    "autoprefixer"
            ]
    },

the deploy would silently fail because "gulp-plumber" would swallow the
error. If I am reading this background material on plumber correctly:

- https://github.com/floatdrop/gulp-plumber
- gulpjs/gulp#91
- https://gist.github.com/floatdrop/8269868

the intent of the plug-in is to monkey patch the Gulp pipe objects such
that an error in one file won't prevent the others from being processed.
We added it in this repo in commit 9200922 (Jan
2016, "Use gulp-plumber for r2 task so that sass-parse errors don't
2abort build process").

In practice, its black magic is causing deploys to fail inscrutably, so
we're dropping it. In the event of an error, let's fail fast instead.
And note, in this concrete instance, I don't think there was even an
error being thrown in the autoprefixer build at all (based on my
`console.log()`-ing around to see at what point it was going off the
rails; the postcss run is finishing just fine.

Note that we could also switch our old v0.6.6 version of this dependency
to the latest, v1.2.1, but there's nothing in the diff that stands out
as being likely to fix this issue, and I'll be much happier if we can
jettison this complexity:

    floatdrop/gulp-plumber@v0.6.6...v1.2.1

Test plan: In portal, in modules/apps/frontend-theme-fjord/frontend-theme-fjord,
turn on autoprefixer with the config mentioned above. Run both `gradlew
clean deploy` and also (the equivalent):

```
yarn run gulp deploy \
  --css-common-path ./build_gradle/frontend-css-common \
  --styled-path ../../frontend-theme/frontend-theme-styled/src/main/resources/META-INF/resources/_styled \
  --unstyled-path ../../frontend-theme/frontend-theme-unstyled/src/main/resources/META-INF/resources/_unstyled
```

and see the build output continue past the "autoprefixer" lines to
finish with:

```
[13:21:39] Finished 'deploy' after 5.27 s
✨  Done in 5.97s.
```

Likewise, see the deployment reflected in the server log:

```
1 theme for fjord-theme is available for use
```

Closes: #274
wincent added a commit to liferay/liferay-js-themes-toolkit that referenced this issue Apr 8, 2019
When deploying a theme using this config in the package.json:

    "liferayTheme": {
            "baseTheme": "styled",
            "distName": "fjord-theme",
            "rubySass": false,
            "version": "7.1",
            "postcss": [
                    "autoprefixer"
            ]
    },

the deploy would silently fail because "gulp-plumber" would swallow the
error. If I am reading this background material on plumber correctly:

- https://github.com/floatdrop/gulp-plumber
- gulpjs/gulp#91
- https://gist.github.com/floatdrop/8269868

the intent of the plug-in is to monkey patch the Gulp pipe objects such
that an error in one file won't prevent the others from being processed.
We added it in this repo in commit 9200922 (Jan
2016, "Use gulp-plumber for r2 task so that sass-parse errors don't
abort build process").

In practice, its black magic is causing deploys to fail inscrutably, so
we're dropping it. In the event of an error, let's fail fast instead.
And note, in this concrete instance, I don't think there was even an
error being thrown in the autoprefixer build at all (based on my
`console.log()`-ing around to see at what point it was going off the
rails; the postcss run is finishing just fine.

Note that we could also switch our old v0.6.6 version of this dependency
to the latest, v1.2.1, but there's nothing in the diff that stands out
as being likely to fix this issue, and I'll be much happier if we can
jettison this complexity:

    floatdrop/gulp-plumber@v0.6.6...v1.2.1

Test plan: In portal, in modules/apps/frontend-theme-fjord/frontend-theme-fjord,
turn on autoprefixer with the config mentioned above. Run both `gradlew
clean deploy` and also (the equivalent):

```
yarn run gulp deploy \
  --css-common-path ./build_gradle/frontend-css-common \
  --styled-path ../../frontend-theme/frontend-theme-styled/src/main/resources/META-INF/resources/_styled \
  --unstyled-path ../../frontend-theme/frontend-theme-unstyled/src/main/resources/META-INF/resources/_unstyled
```

and see the build output continue past the "autoprefixer" lines to
finish with:

```
[13:21:39] Finished 'deploy' after 5.27 s
✨  Done in 5.97s.
```

Likewise, see the deployment reflected in the server log:

```
1 theme for fjord-theme is available for use
```

Closes: #274
wincent added a commit to liferay/liferay-js-themes-toolkit that referenced this issue Apr 8, 2019
This is the 8.x equivalent of:

    #287

When deploying a theme using this config in the package.json:

    "liferayTheme": {
            "baseTheme": "styled",
            "distName": "fjord-theme",
            "rubySass": false,
            "version": "7.1",
            "postcss": [
                    "autoprefixer"
            ]
    },

the deploy would silently fail because "gulp-plumber" would swallow the
error. If I am reading this background material on plumber correctly:

- https://github.com/floatdrop/gulp-plumber
- gulpjs/gulp#91
- https://gist.github.com/floatdrop/8269868

the intent of the plug-in is to monkey patch the Gulp pipe objects such
that an error in one file won't prevent the others from being processed.
We added it in this repo in commit 9200922 (Jan
2016, "Use gulp-plumber for r2 task so that sass-parse errors don't
abort build process").

In practice, its black magic is causing deploys to fail inscrutably, so
we're dropping it. In the event of an error, let's fail fast instead.
And note, in this concrete instance, I don't think there was even an
error being thrown in the autoprefixer build at all (based on my
`console.log()`-ing around to see at what point it was going off the
rails; the postcss run is finishing just fine.

Note that we could also switch our old v0.6.6 version of this dependency
to the latest, v1.2.1, but there's nothing in the diff that stands out
as being likely to fix this issue, and I'll be much happier if we can
jettison this complexity:

    floatdrop/gulp-plumber@v0.6.6...v1.2.1

Test plan: In portal, in modules/apps/frontend-theme-fjord/frontend-theme-fjord,
turn on autoprefixer with the config mentioned above. Run both `gradlew
clean deploy` and also (the equivalent):

```
yarn run gulp deploy \
  --css-common-path ./build_gradle/frontend-css-common \
  --styled-path ../../frontend-theme/frontend-theme-styled/src/main/resources/META-INF/resources/_styled \
  --unstyled-path ../../frontend-theme/frontend-theme-unstyled/src/main/resources/META-INF/resources/_unstyled
```

and see the build output continue past the "autoprefixer" lines to
finish with:

```
[13:21:39] Finished 'deploy' after 5.27 s
✨  Done in 5.97s.
```

Likewise, see the deployment reflected in the server log:

```
1 theme for fjord-theme is available for use
```

Related: #274
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants