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

upgrade to gulp 4 and update others dev dependencies #2151

Merged
merged 4 commits into from
Sep 3, 2019

Conversation

Symro
Copy link

@Symro Symro commented Aug 29, 2019

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Hi I saw your tweet and the issue #2150. I did the migration to Gulp 4. You can test by yourself to check if it's the intended behaviour. It should I did not change that much.

What I did exactly:

  • Updated some dev dependencies used in gulp
  • Changed the babel node modules to be compatible with Gulp V4
  • Updated the babel config inside the package.json
  • Moved Autoprefixer browserlist config from gulpfile to the package.json

My node version: v10.15.4 and npm: 6.9.0. I don't know if the projects now works for "node": ">=8" and "npm": ">=3". Maybe we need to update the engine config aswell.

Cheers ! 🍻

@roblarsen
Copy link
Member

That's great, thanks! I'll test this out.

dot: true

}).pipe(gulp.dest(dirs.dist))
// Include hidden files by default
Copy link
Member

Choose a reason for hiding this comment

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

minor nitpick here - but the indentation is a little bit off. should be 4 spaces (not 6) on lines 124 and 125 and indent of 2 spaces on line 126.

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@coliff
Copy link
Member

coliff commented Sep 2, 2019

great PR! Nice work @Symro. I've tested this on Windows 10 Pro and no issues. Builds fine with no errors and npm run test completes all 41 tests successfully.


gulp.task('default', ['build']);
gulp.task('default', gulp.series('build'))
Copy link
Member

Choose a reason for hiding this comment

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

eslint complains that this should have a semicolon at the end here.

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@roblarsen
Copy link
Member

This is really great. I tested with and it build with no problem.

  • 12.9.1
  • 10.16.3
  • 8.9.4

Thanks for coming through so quickly on this. I am really happy we were able to get this fixed.

@Symro
Copy link
Author

Symro commented Sep 3, 2019

This is really great. I tested with and it build with no problem.

  • 12.9.1
  • 10.16.3
  • 8.9.4

Thanks for coming through so quickly on this. I am really happy we were able to get this fixed.

You're welcome ;), I dit this kind of work a couple of months ago so it was really fun to do it again for an open source project 👍.

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

Successfully merging this pull request may close these issues.

3 participants