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

Option to pass a function instead to tasks #156

Open
shama opened this issue Jul 13, 2013 · 30 comments
Open

Option to pass a function instead to tasks #156

shama opened this issue Jul 13, 2013 · 30 comments
Milestone

Comments

@shama
Copy link
Member

shama commented Jul 13, 2013

Problem
People don't seem to like the watch event. I admit it is a bit cumbersome as the one event that handles all watch targets and is separate from where the watch is configured. It also currently doesn't work well with spawning task runs and with the use case of modifying the config, it doesn't work at all.

Idea
Have tasks accept a function instead:

jshint: {
  all: ['lib/*.js'],
},
watch: {
  target: {
    files: ['lib/*.js'],
    tasks: function(changedFiles) {
      grunt.config('jshint.all', changedFiles);
      grunt.task.run('jshint');
    },
  },
},

Or if you want to spawn:

jshint: {
  all: grunt.option('files') || ['lib/*.js'],
},
watch: {
  target: {
    files: ['lib/*.js'],
    tasks: function(changedFiles) {
      grunt.util.spawn({
        grunt: true,
        args: ['jshint', '--files', changedFiles.join(',')],
      }, function() { });
    },
  },
},

Or if you have some really crazy use case; there won't be a need to request it implemented here as users can package it into their own module:

watch: {
  target: {
    files: ['lib/*.js'],
    tasks: require('my-crazy-task-runner'),
  },
},

This would close #151, #149, #82 and probably a few more.

@rschmukler
Copy link

@shama I think this is an awesome solution.

@ichernev
Copy link
Contributor

that has roughly the same functionality as watch-pre-run event, so +1

@mgol
Copy link
Contributor

mgol commented Jul 17, 2013

There's one problem with this proposal - it relies on the task configuration to have a specified format (i.e. that jshint.all has the list of files directly, without additional options, etc.); also, it won't work with tasks restricted to a target.

@rschmukler
Copy link

@mzgol What about something like: grunt.config('jshint.all.files', changedFiles);? I think you just change where you put them....

@mgol
Copy link
Contributor

mgol commented Jul 17, 2013

@rschmukler What if you changed your configuration to:

grunt.initConfig({
    jshint: {
        all: {
            src: ['app/**/*.js'],
        },
    },
    // ...
});

Would grunt.config('jshint.all.files', changedFiles); still work?

@ichernev
Copy link
Contributor

There is this.files and this.filesSrc that get extracted for you by grunt. If you read the code you'll see the proper way to replace/filter files for tasks.

@mgol
Copy link
Contributor

mgol commented Jul 17, 2013

@ichernev this.files work only inside the task. We're here inside the watch task so we won't get to jshint this.files from here. That's exactly the problem.

@ichernev
Copy link
Contributor

Yes. So I said read the code that extracts files from the config and see how you should change config it so that the code fetches your files, not the one that are configured by default.

@mgol
Copy link
Contributor

mgol commented Jul 17, 2013

@ichernev But this is not my task, I can't modify pre-defined imported tasks like jshint or uglify.

@mattkime
Copy link

I like this example but it could be shorted significantly -

jshint: {
  all: ['lib/*.js'],
},
watch: {
  target: {
    files: ['lib/*.js'],
    tasks: function(changedFiles) {
      grunt.config('jshint.all', changedFiles);
      grunt.task.run('jshint');
    },
  },
},

this is rather roundabout way to say "run jshint.all on the changedFiles"

it would be nice if all the targets could have an option that stated "watch: true" or something similar. another idea - being able to simply run jshint.all on a set of files.

need to get my hands dirty to see what works best.

@shama
Copy link
Member Author

shama commented Jul 21, 2013

@mattkime That is required for jshint in grunt v0.4 because its a multitask. It will be simpler to just run tasks on sets of files going forward. See https://github.com/node-task/spec/wiki

@mlmorg
Copy link

mlmorg commented Jul 25, 2013

I think this is a great idea. I've been playing around with it locally on a project and it works well. The main issue is that it can indeed be a bit verbose; for example, if I am linting and compiling some coffeescript files:

coffeelint:
  all: ['app/scripts/**/*.coffee']
coffee:
  dev:
    files: [
      expand: true
      cwd: 'app/scripts'
      src: '**/*.coffee'
      dest: '.tmp'
      ext: '.js'
    ]
watch:
  options:
    spawn: false
  coffee:
    files: ['app/scripts/**/*.coffee']
    tasks: (changed) ->
      # Lint config
      grunt.config('coffeelint.all', changed)

      # Compile config
      files = grunt.config('coffee.dev.files')
      files[0].src = changed.map (file) -> path.relative(files[0].cwd, file)
      grunt.config('coffee.dev.files', files)

      # Run tasks
      grunt.task.run(['coffeelint', 'coffee:dev'])

Another option may be for grunt-contrib-watch to handle this logic itself. Unfortunately, this puts a lot of burden on to grunt-contrib-watch because it would need to support all of the different file mapping formats — both to determine what part of the tasks configuration to edit as well as editing it correctly (like the globbing pattern change I had to do above). It would make for a much cleaner config file, though.

@shama
Copy link
Member Author

shama commented Jul 25, 2013

I don't mind if the config is verbose. You can always throw the logic you want into a module:

watch: {
  target: {
    files: ['lib/*.js'],
    tasks: require('my-crazy-task-runner'),
  },
},

@minhur
Copy link

minhur commented Aug 6, 2013

I would love to see this feature.
I'm currently using watch event with nospawn: true, and although it works well in mac, the change event (not my custom event) often doesn't trigger in windows.

@tardyp
Copy link

tardyp commented Aug 11, 2013

Please see this PR for an alternate method, using filter option of grunt files:

#180

this avoids crazy configuration for a feature as fundamental as incremental build.

@shama
Copy link
Member Author

shama commented Aug 11, 2013

@tardyp Could you elaborate on why this method is "crazy"? Having the watch simply running a callback function upon file change seems like the most sane way for those requesting this feature.

Which btw, is not incremental builds.

@tardyp
Copy link

tardyp commented Aug 12, 2013

Well, this is at least a first step to not recompile the whole stuff, when I save one file. One can indeed argue that in some more complex setup, incremental build is more complex.

For crazy, you write it first with my-crazy-task-runner :-). For me the solution that is proposed requires a bit too much understanding of grunt internals. I think it would be better to do more stuff automatically. I think the hasChanged filter in grunt.file is more understandable from a user point of view.

Also the promise of GruntFile is to be nearly configuration only, there should be no need to have the keyword "function" in a GruntFile.js, all logic should be handled by plugins installed via npm (i.e user should not have to write custom tasks for 99% of projects)

Last, I think that it is better to have the tasks configs be aware of watch rather than watch configs to be aware of the task ( though, this last argument is very much debatable opinion)

Actually, I would suggest to have both method implemented

@shama
Copy link
Member Author

shama commented Aug 12, 2013

The intent of this method is to give users access to the internals of this task. So they can run their-crazy-task-runner without needing me to implement a crazy task runner here.

We already have an automatic method implemented, tasks and files config.

Gruntfiles are not exclusively config only otherwise they would be JSON files. They are javascript. There are plenty of reasons to use functions within gruntfiles and I highly encourage it. In fact in Grunt v0.5, Gruntfiles will more resemble regular node modules than configuration files.

@ichernev
Copy link
Contributor

I agree with @tardyp that support for running a task only on subset of files (namely the changed ones) should be easier for the end user, meaning some more code in grunt-contrib-watch, grunt, or something else that is part of the ecosystem. You can classify tasks in 3 categories, based on the files they operate on:

  1. source files only (flat) -- jshint is like that. It runs on a set of files and doesn't produce any new files
  2. source and destination (fixed) -- concat is like that. It has a bunch of sources -> destination pairs, and when a single source file changes you need to recompile destination
  3. sources (deep) and destination -- requirejs and sass are like that. You give a bunch of source files and a destination, but this time each source can take more sources into account to produce destination (because of import/require calls and the like)

For the first category, plugin authors must use this.fileSrc, which has defined semantics, so you can write a tool that gets all sources and produces a limited set of sources, fully automated.

For the first category, plugin authors must use this.files, which again, has defined semantics, so you can
automate its filtering.

The third category is more tricky, but if plugin authors exposed a way to read this dynamic dependency graph, it would be possible to filter automatically. For example sass can record a map of file to dependencies under grunt.config after each run, so another task can traverse all toplevel targets and see if they depend (directly or indirectly) on changed files.

Related sass issue: sass/sass#809 (includes a one liner that fetches deps).
r.js already outputs build.txt in a format that's easy to parse.

So with a bit of help from the corresponding grunt tasks, grunt-restrict can work for the third case too (now it mostly handles the first and second one). It currently uses a pre-run event from grunt-watch (implemented in my fork), but this PR will fix this issue.

The main point is that the whole auto-filtering is totally doable and not in a terribly hacky way, so I don't understand why is it not part of core, or at least why maintainers fail to acknowledge the importance of this use case and have a discussion about ways to integrate this into the ecosystem (by any means necessary).

@mattkime
Copy link

+1 @ichernev

@shama
Copy link
Member Author

shama commented Aug 13, 2013

Anything implemented in the core must be generic and work for all tasks

We can implement dependency graph support for requirejs, sass... but what about browserify, less, handlebars, jst, concat, stylus, rework, docpad, coffeescript, typescript and all those other tasks that can include files? Not to mention when a build flow includes 2 or more of these task steps and their dependency graphs must work together.

Do you really think the Grunt core can support dependency graphs for all tasks? If so then please send us a patch. Or even better, write that library so the entire node community can benefit from it.

If you want to filter files based on modification date, done. Use the filter option with a custom function.

If you want to filter files based on being watched, done. Use the watch event, use gaze, core fs.watch or one of the many other watch wrappers.

We've had many many discussions about this and we are still completely open to suggestions. If a solution doesn't work for all tasks then it doesn't belong in the core.

@shama
Copy link
Member Author

shama commented Aug 13, 2013

@ichernev Unless you're suggesting we add an API in the Grunt core that allows task authors to specify their own dependency graph (determined completely by the task author)... if so, I think that is a really good idea.

@ichernev
Copy link
Contributor

The statement about "all tasks" is just plain wrong. There is this.fileSrc in core. Does it work for all tasks? No. It works for the tasks that only have source files. Same for this.files. Grunt provides tools that makes it easy to write tasks. If you want you can use them.

I'm pretty sure that the demand for partial execution is big enough for the main projects to implement dep storage in grunt.config. It won't happen overnight, but it might catch on.

About two stage tasks -- I'm already having 3 stage tasks in my config and they work just fine as long as you put each individual stage in watch and trigger the next one (which makes sense). So you only need to take care of one stage at a time.

I can make a proof-of-concept requirejs/sass fork that store deps and then a tasks that restricts based on that. The core support for that may vary from put your dependencies in grunt.config.name.deps in this form so that others can use it from there to a complete partial execution solution that works for tasks that use this.fileSrc, this.files and deps, and a watch task that understands that.

@shama
Copy link
Member Author

shama commented Aug 13, 2013

Yes, this.filesSrc and this.files does work in all tasks. Just because an API isn't used in all tasks doesn't mean it doesn't work.

Having a solution that only works for requirejs/sass is not working in all tasks.

If you're suggesting we add a dependency graph API into grunt... I am +1.
If you're going to support dependency graphs for just requirejs/sass... I am -1.

@cgross
Copy link

cgross commented Feb 24, 2014

+1 to the overall idea of allow tasks to be a function.

I think it would be nicer (and fit with #293) to allow the function to return a list of tasks to be executed rather than having to execute the tasks itself. Would keep it a little less verbose.

If a PR is welcome, I would be willing to work on it.

@shama
Copy link
Member Author

shama commented Feb 24, 2014

FWIW I have it implemented here in grunt-next which will replace this task (either by a new future version grunt-contrib-watch or integrated back into grunt's core; the team is still undecided). Either case I'll implement here in a version compatible with the current version of grunt.

@cgross
Copy link

cgross commented Feb 24, 2014

Sweet. Tx.

@kanecko
Copy link

kanecko commented Apr 24, 2014

Has anyone got the delay on livereload to work?

@hellboy81
Copy link

hellboy81 commented Aug 11, 2016

I have script which should be executed for each added or changed file.

I have 3 theoretical possibilities:

  • execute script for every changed file from grunt watcher. Full path of each changed/added file should be provided
  • Build CLI parameters with all changed files and execute script once with this parameters
  • Do not use grunt and grunt-contib-watch. Your script should implement you own watcher. This is not Unix-way-style (my script should do 2 jobs: watching files in directory and handle this files), but due on some grunt-contib-watch restrictions it is not possible to implement feature in 2 previous ways.

@durgesh1189
Copy link

I want to use Grunt Behat Parallel option, but I don't want it to write it in initConfig to execute it default. I am totally new to Grunt, can somebody please help.

here is example which I dont want to use in initConfig

grunt.initConfig({
behat: {
example: {
options: {
output: false,
failOnUndefined: false,
failOnFailed: false
},
cmd: './bin/behat',
features: 'features/',
flags: '-f pretty'
},
example2: {
features: 'features/',
}
}
});

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

No branches or pull requests