linemanjs / lineman

Lineman helps you build fat-client JavaScript apps. It produces happiness by building assets, mocking servers, running specs on every file change
MIT License
1.18k stars 80 forks source link

Grunt Contrib Watch #234

Closed davemo closed 10 years ago

davemo commented 10 years ago

This upgrades us away from grunt-nospawn and to grunt-contrib-watch which seems to be working properly as of 0.6.0.

I've set spawn: false in the options, but purposely left out livereload as it is configurable in lineman-livereload which is only usable once this is merged.

If anyone has bandwidth to test, you can clone https://github.com/davemo/lineman-livereload-testing, npm install and lineman run. If you have the Chrome live reload extension, enable it once lineman is running and you'll get live reloaded css/js changes.

Would love some testing to see if this is mergeworthy in any larger Lineman projects you guys have available. /cc @searls @jasonkarns @jayharris

searls commented 10 years ago

What's everyone's feelings on this? I still haven't tested it but I've felt for a long time like we should invest the time in a directory watch approach based on @davemo's research in the past.

jayharris commented 10 years ago

tl;dr; This is a good thing. +1. Directory monitoring is a good thing. +1. But do this first until we have time to do directory monitoring.

detail:

I've been running grunt-contrib-watch for some time without issue. It seems to handle a much higher file handle count than nospawn when run against large projects.

I agree that the Directory approach is warranted, but there's a lot of value in both. The problem with Gaze (which powers grunt-contrib-watch, gulp, and others) is that it doesn't monitor directories, even though Glob fully supports directory patterns. This should be a configurable thing, whether you want to supply /js/**/*.js to monitor all of the JS files themselves, or /js/**/ if you want to monitor only the directories for changes. (Changes to the files within the directory will still trigger a change on the directory.)

If effort is to be put in for Directory monitoring, we should be good open-source citizens and help gaze rather than home-growing our own.

searls commented 10 years ago

@jayharris @davemo have either of you been running this particular fork of lineman? I don't want to merge-and-pray that it'll just work, because that'd mark, like, the third time we'll have done that only to revert it once we find bugs.

davemo commented 10 years ago

I ran it through some of my smaller projects without issue but the testing was not extensive. On Apr 12, 2014 11:26 AM, "Justin Searls" notifications@github.com wrote:

@jayharris https://github.com/jayharris @davemohttps://github.com/davemohave either of you been running this particular fork of lineman? I don't want to merge-and-pray that it'll just work, because that'd mark, like, the third time we'll have done that only to revert it once we find bugs.

Reply to this email directly or view it on GitHubhttps://github.com/linemanjs/lineman/pull/234#issuecomment-40283369 .

searls commented 10 years ago

By the way, I agree with Jay's reasoning

On Sat, Apr 12, 2014 at 11:29 AM, David Mosher notifications@github.com wrote:

I ran it through some of my smaller projects without issue but the testing was not extensive. On Apr 12, 2014 11:26 AM, "Justin Searls" notifications@github.com wrote:

@jayharris https://github.com/jayharris @davemohttps://github.com/davemohave either of you been running this particular fork of lineman? I don't want to merge-and-pray that it'll just work, because that'd mark, like, the third time we'll have done that only to revert it once we find bugs.

Reply to this email directly or view it on GitHubhttps://github.com/linemanjs/lineman/pull/234#issuecomment-40283369 .


Reply to this email directly or view it on GitHub: https://github.com/linemanjs/lineman/pull/234#issuecomment-40283431

searls commented 10 years ago

Yeah I just tested locally and I'm :-1: so far, I'm seeing that its performance is about 4 to 5 times slower in my current client project

searls commented 10 years ago

Yeah it's obsence how much slower grunt-contrib-watch is than grunt-watch-nospawn. I'm seeing 100-300ms response times for CoffeeScript & template builds in grunt-watch-nospawn@0.0.4, and 1.2-1.7s feedback loops on grunt-contrib-watch@06.1

un33k commented 10 years ago

I have tested this and it works well. No benchmark though!

Would it be possible to enable grunt-watch-nospawn vs grunt-contrib-watch based on detection of lineman-livereload, only if performance is an issue as per @searls comment?

searls commented 10 years ago

Maybe. I'm more interested in speed of feedback than live reload. Where I really want to invest time is in linemanjs/grunt-watch-fireworm

On Tue, Apr 29, 2014 at 12:10 AM, Val Neekman notifications@github.com wrote:

I have tested this and it works well. No benchmark though!

Would it be possible to enable grunt-watch-nospawn vs grunt-contrib-watch based on detection of lineman-livereload, only if performance is an issue as per @searls comment?

Reply to this email directly or view it on GitHub: https://github.com/linemanjs/lineman/pull/234#issuecomment-41640113

jewelsjacobs commented 10 years ago

Does this include livereload capabilities making lineman-livereload obsolite?

Oh I guess not.

I really wish livereload were easier to use with lineman, ie. without a lineman plugin and branch reference just a watch config plugin like

module.exports = (lineman) ->
  config:
    watch:
      options:
        livereload: true
searls commented 10 years ago

Hey @jewelsjacobs -- yeah, unfortunately we still don't have a good answer to either (1) file watching or (2) live reload.

As described above, the I/O performance of grunt-contrib-watch is very, very slow compared to what the older fork that Lineman uses. Similarly, the fork we do use continues to have problems, and we'd like to invest the time to move Lineman to https://github.com/airportyh/fireworm

If we move to fireworm then I think it'd probably be trivial for us to add livereload support. But in the meantime, perhaps you or someone from your team could investigate how to add livereload support to grunt-watch-nospawn? I'd happily merge in a PR that didn't degrade performance (further) https://github.com/testdouble/grunt-watch-nospawn

searls commented 10 years ago

Hey @jewelsjacobs -- I added livereload support to lineman@0.33.3 by updating grunt-watch-nospawn. This probably invalidates your need for a separate plugin.

davemo commented 10 years ago

I'm going to resurrect this again, I've made a branch to run a current client project off that was running up against the EMFILE issue. The switch here improves the logspam that was introduced somewhere between 0.29.x and HEAD; despite the small performance losses I think this is a huge quality of life improvement. As a tertiary benefit It would also enable simpler live-reload configuration for those who want it. Going to leave as a branch for now for those who want to run off it, you can update your package.json as follows:

  "devDependencies": {
    "lineman": "git://github.com/linemanjs/lineman.git#grunt-contrib-watch"
   }
searls commented 10 years ago

Rather than fork lineman, why not patch it so that a local install of contrib-watch overrides nospawn?

We already do this for all other task modules, this would just require a hardcoding of a particular mapping. 

Makes sense to me since the config is compatible

On Thu, Aug 21, 2014 at 11:14 AM, David Mosher notifications@github.com wrote:

I'm going to resurrect this again, I've made a branch to run a current client project off that was running up against the EMFILE issue. The switch here improves the logspam that was introduced somewhere between 0.29.x and HEAD; despite the small performance losses I think this is a huge quality of life improvement. As a tertiary benefit It would also enable simpler live-reload configuration for those who want it. Going to leave as a branch for now for those who want to run off it, you can update your package.json as follows:

  "devDependencies": {
    "lineman": "git://github.com/linemanjs/lineman.git#grunt-contrib-watch"
   }

Reply to this email directly or view it on GitHub: https://github.com/linemanjs/lineman/pull/234#issuecomment-52934392