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

Switch from Grunt to Gulp #1138

Closed
wants to merge 73 commits into
base: master
from

Conversation

Projects
None yet
@retlehs
Member

retlehs commented Aug 24, 2014

edit: moved to #1251

ref #1215 for more information on the new manifest.json asset pipeline

@retlehs retlehs referenced this pull request Aug 24, 2014

Closed

Switch from Grunt to Gulp #1110

kalenjohnson and others added some commits Sep 5, 2014

@JulienMelissas

This comment has been minimized.

Show comment
Hide comment
@JulienMelissas

JulienMelissas Sep 7, 2014

Member

Hey all...

I've been doing some Gulp.js testing this weekend, and while it's working really well, it looks like when there's an error, the gulp console has to be quit and then restarted. Also, on my mac, it doesn't notify the terminal, or the user, like grunt does.

I'm wondering if maybe we should add gulp-notify and/or gulp-plumber to help handle error handling?

Other than that, it's working pretty well, and it IS noticeably faster.

Member

JulienMelissas commented Sep 7, 2014

Hey all...

I've been doing some Gulp.js testing this weekend, and while it's working really well, it looks like when there's an error, the gulp console has to be quit and then restarted. Also, on my mac, it doesn't notify the terminal, or the user, like grunt does.

I'm wondering if maybe we should add gulp-notify and/or gulp-plumber to help handle error handling?

Other than that, it's working pretty well, and it IS noticeably faster.

austinpray added some commits Sep 12, 2014

Enhance Gulp dependency compilation
Gulp automatically grabs all of the dependencies listed as main files in
`bower.json` except for jQuery and modernizr

Deletes plugin directory, bad development pattern. Doesn't make sense
with the existence of `vendor` directory anyway.

Concat all of the js in the js directory except vendor and compiled
files
Adds `gulp images`
Lossless image optimization including pngcrush(1)
@austinpray

This comment has been minimized.

Show comment
Hide comment
@austinpray

austinpray Sep 12, 2014

Member

Just sent over a PR that enhances the gulp functionality. I tried not to change too much before we discuss.

There are some things I have identified about your build process that are very odd and perhaps we should take this opportunity to correct while we are introducing breaking changes:

Vendoring

"You keep using that word. I do not think it means what you think it means."
Currently the directory structure looks like this:

  • assets
    • js
      • vendor
    • vendor (bower)

and presumably if you wanted to add some vendor'd css

  • assets
    • js
      • vendor
    • css
      • vendor
    • vendor (bower)

The whole point of vendoring is so third-party code is clearly separated from first-party source code. The directory structure should look like this:

  • bower_components
  • vendor
    • js
    • css
  • assets
    • js
    • css

Where vendor is ideally completely blank. If you are checking things into a vendor folder it's because you absolutely cannot use your package manager to grab them or you did some monkey patches or something.

Mixing compiled files with src files

My primary wheelhouse is building javascript apps so this sticks out like a sore thumb to me. There needs to be a very clear build process. Just look at the amount of gymnastics you have to do to make sure scripts.min.js does not accidentally get jshinted or concatted in the build process. Also, what if I want to make clean my build? What if something went wrong and I want to completely refresh my compiled files with a rm -r dist? Can't be done right now, you would have to manually go through and delete the files. Also novice users might be very confused as to why when they run gulp build all of their changes to script.js are deleted. Also think about running a grep on the assets/js directory to do some refactoring, it would pull up a bunch of junk. All in all this is bad.

Deployment

How exactly do you expect to get these theme files to the server? Obviously the compiled files are not checked into source control, so are you manually uploading them? This doesn't work with capistrano, which is what bedrock uses. This means bedrock-ansible + bedrock + roots is DOA without checking compiled files into source control.

The final layout should look like this:

  • bower_components
  • vendor
    • js
    • css
  • assets
    • js
    • css
  • dist (bower + vendor + assets compiled)
    • js
    • css

and then use a capistrano task to rsync dist on every deploy. This avoids checking compiled files into the source control and all in all just makes sense.

@swalkinshaw

Member

austinpray commented Sep 12, 2014

Just sent over a PR that enhances the gulp functionality. I tried not to change too much before we discuss.

There are some things I have identified about your build process that are very odd and perhaps we should take this opportunity to correct while we are introducing breaking changes:

Vendoring

"You keep using that word. I do not think it means what you think it means."
Currently the directory structure looks like this:

  • assets
    • js
      • vendor
    • vendor (bower)

and presumably if you wanted to add some vendor'd css

  • assets
    • js
      • vendor
    • css
      • vendor
    • vendor (bower)

The whole point of vendoring is so third-party code is clearly separated from first-party source code. The directory structure should look like this:

  • bower_components
  • vendor
    • js
    • css
  • assets
    • js
    • css

Where vendor is ideally completely blank. If you are checking things into a vendor folder it's because you absolutely cannot use your package manager to grab them or you did some monkey patches or something.

Mixing compiled files with src files

My primary wheelhouse is building javascript apps so this sticks out like a sore thumb to me. There needs to be a very clear build process. Just look at the amount of gymnastics you have to do to make sure scripts.min.js does not accidentally get jshinted or concatted in the build process. Also, what if I want to make clean my build? What if something went wrong and I want to completely refresh my compiled files with a rm -r dist? Can't be done right now, you would have to manually go through and delete the files. Also novice users might be very confused as to why when they run gulp build all of their changes to script.js are deleted. Also think about running a grep on the assets/js directory to do some refactoring, it would pull up a bunch of junk. All in all this is bad.

Deployment

How exactly do you expect to get these theme files to the server? Obviously the compiled files are not checked into source control, so are you manually uploading them? This doesn't work with capistrano, which is what bedrock uses. This means bedrock-ansible + bedrock + roots is DOA without checking compiled files into source control.

The final layout should look like this:

  • bower_components
  • vendor
    • js
    • css
  • assets
    • js
    • css
  • dist (bower + vendor + assets compiled)
    • js
    • css

and then use a capistrano task to rsync dist on every deploy. This avoids checking compiled files into the source control and all in all just makes sense.

@swalkinshaw

@austinpray

This comment has been minimized.

Show comment
Hide comment
@austinpray

austinpray Sep 12, 2014

Member

Oh and also definitely recommend ditching gulp-modernizr for now. It's a cool idea but it's absolutely not ready for primetime. It's based off of an unstable build of modernizr (3.0-beta) and it requires grunt dependencies

screen shot 2014-09-12 at 12 18 59 am

Member

austinpray commented Sep 12, 2014

Oh and also definitely recommend ditching gulp-modernizr for now. It's a cool idea but it's absolutely not ready for primetime. It's based off of an unstable build of modernizr (3.0-beta) and it requires grunt dependencies

screen shot 2014-09-12 at 12 18 59 am

@austinpray

This comment has been minimized.

Show comment
Hide comment
@austinpray

austinpray Sep 14, 2014

Member

Also we should be using csso instead of minifyCSS

http://bem.info/tools/optimizers/csso/

Member

austinpray commented Sep 14, 2014

Also we should be using csso instead of minifyCSS

http://bem.info/tools/optimizers/csso/

@QWp6t

This comment has been minimized.

Show comment
Hide comment
@QWp6t

QWp6t Sep 14, 2014

Member

I agree with csso recommendation. I like it a lot more, in particular because I can set it to not restructure the CSS, which is what prompted me to find an alternative to cssmin in the first place.

Member

QWp6t commented Sep 14, 2014

I agree with csso recommendation. I like it a lot more, in particular because I can set it to not restructure the CSS, which is what prompted me to find an alternative to cssmin in the first place.

@borodean

This comment has been minimized.

Show comment
Hide comment
@borodean

borodean commented Sep 17, 2014

retlehs and others added some commits Dec 10, 2014

Update and tidy README
Removes references to defunct modernizr build generator
Removes repetition
Gulp -> gulp
https://github.com/gulpjs/gulp/blob/master/docs/FAQ.md#is-it-gulp-or-gulp
Removes references to Sass fork, it is dead
Removes "Organized file and template structure" as a feature
Some opinionated usage changes in the Contributing section
Adds my twitter to Readme so I can be reached for gulp stuff
Pimps bedrock
Merge pull request #1240 from austinpray/gulp
Have Travis run JSHint and verify that the build process runs successfully
@austinpray

This comment has been minimized.

Show comment
Hide comment
@austinpray

austinpray Jan 3, 2015

Member

oops ( ͡° ͜ʖ ͡°)

Member

austinpray commented on README.md in 6a7764f Jan 3, 2015

oops ( ͡° ͜ʖ ͡°)

@retlehs retlehs closed this Jan 10, 2015

@retlehs retlehs referenced this pull request Jan 10, 2015

Merged

Sage 8.0.0 #1251

9 of 9 tasks complete

@retlehs retlehs deleted the gulp branch Jan 10, 2015

@retlehs

This comment has been minimized.

Show comment
Hide comment
@retlehs
Member

retlehs commented on lib/assets.php in 3a4ae77 Jan 11, 2015

@alexciarlillo

This comment has been minimized.

Show comment
Hide comment
@alexciarlillo

alexciarlillo Jan 14, 2015

I'm still catching up to all these rapid fire changes and switching things over to gulp... but does this not output the wiredep-ed main.less to the dist folder, while the globs being processed by cssTasks are coming from the assets folder? I tried running the build after this change but my main.css was not being built from the wiredep-ed main.less, rather the original.

alexciarlillo commented on b9722e9 Jan 14, 2015

I'm still catching up to all these rapid fire changes and switching things over to gulp... but does this not output the wiredep-ed main.less to the dist folder, while the globs being processed by cssTasks are coming from the assets folder? I tried running the build after this change but my main.css was not being built from the wiredep-ed main.less, rather the original.

This comment has been minimized.

Show comment
Hide comment
@austinpray

austinpray Jan 14, 2015

Member

Haha that's exactly what it does. It is supposed to be pointed at path.src. This whole task is rewritten on my fork that I am merging in though.

Member

austinpray replied Jan 14, 2015

Haha that's exactly what it does. It is supposed to be pointed at path.src. This whole task is rewritten on my fork that I am merging in though.

This comment has been minimized.

Show comment
Hide comment
@austinpray
Member

austinpray replied Jan 15, 2015

@guix77

This comment has been minimized.

Show comment
Hide comment
@guix77

guix77 Jan 26, 2015

Contributor

The logic here for Sage seems to favorise Bower and I'm totally for it in general.

I was updating an old Roots-based theme before the Modernizr custom build and I was asking myself : why build a custom Moderniz and save a few kB (half, maybe), instead of using its main CDN with the latest complete build... so that people could have it in their cache already.

Would it be correct to consider Modernizr like a "WP essential" (and above WP..) like jQuery, and grab it through a CDN ?

If not, why not merging the custom build into scripts.min.js ?

... just trying to save 1 query for our mobile users there ;)

Keep up the awesome work !

Contributor

guix77 commented on 5389bd6 Jan 26, 2015

The logic here for Sage seems to favorise Bower and I'm totally for it in general.

I was updating an old Roots-based theme before the Modernizr custom build and I was asking myself : why build a custom Moderniz and save a few kB (half, maybe), instead of using its main CDN with the latest complete build... so that people could have it in their cache already.

Would it be correct to consider Modernizr like a "WP essential" (and above WP..) like jQuery, and grab it through a CDN ?

If not, why not merging the custom build into scripts.min.js ?

... just trying to save 1 query for our mobile users there ;)

Keep up the awesome work !

This comment has been minimized.

Show comment
Hide comment
@austinpray

austinpray Jan 26, 2015

Member

Good comments, thanks for reviewing the code. It really helps us a lot.

using its main CDN with the latest complete build

Assets definitely need to be version locked. I'm pretty sure you can do this with the CDN hosted version but having the CDN auto-update modernizr for you is asking for trouble. Just wanted to specify that requirement.

If not, why not merging the custom build into scripts.min.js ?

My only concern is that if a plugin specifies modernizr as a dependency and it needs to jump to the header or something dumb like that. Idk enough about caveats with plugins but @Foxaii @swalkinshaw @retlehs can probably help me understand.

Member

austinpray replied Jan 26, 2015

Good comments, thanks for reviewing the code. It really helps us a lot.

using its main CDN with the latest complete build

Assets definitely need to be version locked. I'm pretty sure you can do this with the CDN hosted version but having the CDN auto-update modernizr for you is asking for trouble. Just wanted to specify that requirement.

If not, why not merging the custom build into scripts.min.js ?

My only concern is that if a plugin specifies modernizr as a dependency and it needs to jump to the header or something dumb like that. Idk enough about caveats with plugins but @Foxaii @swalkinshaw @retlehs can probably help me understand.

This comment has been minimized.

Show comment
Hide comment
@guix77

guix77 Jan 26, 2015

Contributor

So there are 2 points :

  1. general web app performance : using a CDN with the whole Modernizr, or a local custom Modernizr ;
  2. WP-specific (main goal of Sage) : putting Modernizr alone in the header for the plugins that require it.

So why not using a CDN Modernizr .js in HEAD ?

Contributor

guix77 replied Jan 26, 2015

So there are 2 points :

  1. general web app performance : using a CDN with the whole Modernizr, or a local custom Modernizr ;
  2. WP-specific (main goal of Sage) : putting Modernizr alone in the header for the plugins that require it.

So why not using a CDN Modernizr .js in HEAD ?

This comment has been minimized.

Show comment
Hide comment
Member

retlehs replied Jan 26, 2015

This comment has been minimized.

Show comment
Hide comment
@austinpray

austinpray Jan 26, 2015

Member

Whatever we do as far as CDNs go:

We need to implement a way to reconcile the bower included version of a dependency (jQuery) with the CDN version of it. If I include version 1.11.1 of jQuery in my bower.json, then the CDN needs to be set to load cdn.google.com/whatever/jquery/1.11.1/jquery.js.

Member

austinpray replied Jan 26, 2015

Whatever we do as far as CDNs go:

We need to implement a way to reconcile the bower included version of a dependency (jQuery) with the CDN version of it. If I include version 1.11.1 of jQuery in my bower.json, then the CDN needs to be set to load cdn.google.com/whatever/jquery/1.11.1/jquery.js.

This comment has been minimized.

Show comment
Hide comment
@QWp6t

QWp6t Jan 30, 2015

Member

I think we can forego the html5shiv entirely and drop support for IE 8. Users of IE 8 are used to pages looking like shit because everyone has dropped support for IE 8, including the upcoming version of Bootstrap.

Android 2.3 native browser support also seems arbitrary since it's used by such a small fraction of users, but that's unrelated to modernizr and html5shiv so I won't dwell on that here.

Member

QWp6t replied Jan 30, 2015

I think we can forego the html5shiv entirely and drop support for IE 8. Users of IE 8 are used to pages looking like shit because everyone has dropped support for IE 8, including the upcoming version of Bootstrap.

Android 2.3 native browser support also seems arbitrary since it's used by such a small fraction of users, but that's unrelated to modernizr and html5shiv so I won't dwell on that here.

This comment has been minimized.

Show comment
Hide comment
@austinpray

austinpray Jan 30, 2015

Member

I think we can forego the html5shiv entirely and drop support for IE 8.

By loading modernizr in the footer we have already dropped support for IE 8. As such: if we aren't obnoxious about it, I am ok with not jumping through hoops to support IE 8. If you have a project that requires IE 8 you can literally just copy paste this into your head:

<!--[if lt IE 9]>
  <script src="https://cdnjs.cloudflare.com/ajax/libs/html5shiv/3.7.2/html5shiv.min.js"></script>
<![endif]-->

And you are back in business.

Member

austinpray replied Jan 30, 2015

I think we can forego the html5shiv entirely and drop support for IE 8.

By loading modernizr in the footer we have already dropped support for IE 8. As such: if we aren't obnoxious about it, I am ok with not jumping through hoops to support IE 8. If you have a project that requires IE 8 you can literally just copy paste this into your head:

<!--[if lt IE 9]>
  <script src="https://cdnjs.cloudflare.com/ajax/libs/html5shiv/3.7.2/html5shiv.min.js"></script>
<![endif]-->

And you are back in business.

This comment has been minimized.

Show comment
Hide comment
@retlehs

retlehs Jan 30, 2015

Member

no modernizr in the head. i'll mention @austinpray's method above in the new docs for sage for those that need it.

Member

retlehs replied Jan 30, 2015

no modernizr in the head. i'll mention @austinpray's method above in the new docs for sage for those that need it.

This comment has been minimized.

Show comment
Hide comment
@JulienMelissas

JulienMelissas Jan 30, 2015

Member

👍 for dropping official support of IE8, it's time.

Member

JulienMelissas replied Jan 30, 2015

👍 for dropping official support of IE8, it's time.

This comment has been minimized.

Show comment
Hide comment
@austinpray
Member

austinpray replied Jan 31, 2015

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