Skip to content

Upgrade gulp v4 #6143

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

Merged
merged 3 commits into from
Aug 22, 2018
Merged

Upgrade gulp v4 #6143

merged 3 commits into from
Aug 22, 2018

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jul 24, 2018

Summary

Update to Gulp v4 and drop deprecated gulp-util dependency.
Also, it drops gulp-watch as it is now integrated within the gulp API.

Motivation

This PR is motivated by the warnings showing up when performing yarn install on this repo. Also, it removes lodash v1.0.2 dependency which is using the deprecated Buffer constructor.

@buildsize
Copy link

buildsize bot commented Jul 24, 2018

File name Previous Size New Size Change
yarn-[version].noarch.rpm 910.47 KB 915.03 KB 4.56 KB (1%)
yarn-[version].js 3.98 MB 4.01 MB 32.75 KB (1%)
yarn-legacy-[version].js 4.14 MB 4.17 MB 32.76 KB (1%)
yarn-v[version].tar.gz 915.78 KB 920.65 KB 4.87 KB (1%)
yarn_[version]all.deb 667.68 KB 673.32 KB 5.63 KB (1%)

gulp.task('build', () =>
build('lib', babelRc.env[majorVer >= 5 ? 'node5' : 'pre-node5'])
);

gulp.task('watch', ['build'], () => {
watch('src/**/*', () => {
gulp.start('build');
Copy link

@jacobq jacobq Aug 2, 2018

Choose a reason for hiding this comment

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

I'm not a gulp expert by any means, but it seems like we're doing gulp.task('build') a lot in here. Is there a good way to clean that up a bit so it's more terse? (e.g. const buildTask = gulp.task('build')) Or is this just the standard/canonical/usual/conventional way to do it?

Copy link

Choose a reason for hiding this comment

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

Looking at a sample in their README, it says, "You can still use gulp.task to expose tasks", but I'm not sure if they're implying that one should or shouldn't.

gulp.task('build', build);

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it matters whether tasks are exported using gulp.task or by exporting them from the module, it's just a matter of style.

As for the repeated use of gulp.task('build'), it looks like the best way to avoid that is to just use the function reference instead. i.e. gulp.task('default', build)

That can't be done as things are currently, because build is a helper function. The actual build task is an anonymous function that calls the build function. I see no reason not to merge those two functions into one. After that, instances of gulp.task('build') could be replaced with build.

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

The readability of this script could be improved, but I still think it's pretty decent. It solves the problem.

aduh95 and others added 3 commits August 14, 2018 00:47
@arcanis arcanis merged commit 3838739 into yarnpkg:master Aug 22, 2018
@arcanis
Copy link
Member

arcanis commented Aug 22, 2018

Thanks! 👍

@aduh95 aduh95 deleted the upgrade-gulp-v4 branch August 22, 2018 20:09
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

Successfully merging this pull request may close these issues.

4 participants