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

watch Command - watch files/dirs and rerun task on change #33

Closed
wants to merge 21 commits into from

Conversation

flxwu
Copy link

@flxwu flxwu commented Jun 5, 2018

Resolves #9

Example

Run task `lint` before this and watch `*.js` 

will rerun lint everytime a JavaScript file is changed

lib/index.js Outdated
function checkTypes(task, types) {
return types.some(type => type === task.type)
}

Copy link
Contributor

@zephraph zephraph Jun 6, 2018

Choose a reason for hiding this comment

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

Why pull this out? I don't disagree with the change, just trying to understand.

Copy link
Author

Choose a reason for hiding this comment

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

Because it's needed in the MaidRunner 😄

.on('change', path => runner.runFile(this.getTaskName(path)))
.on('unlink', path => runner.runFile(this.getTaskName(path)))
.on('add', path =>
this.getTaskNames(path).forEach(taskName => runner.runFile(taskName))
Copy link
Contributor

Choose a reason for hiding this comment

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

synchronous forEach which executes async runFile? Ouch.

lib/parseMarkdown.js Outdated Show resolved Hide resolved
@flxwu
Copy link
Author

flxwu commented Jun 6, 2018

@olstenlarck just made the task executions asynchronous :)

)
) + LAZY,
capture(and(space, 'and', space, 'watch', space, capture(extra(ANY)))) + LAZY,
wildcard(SPACE)
Copy link
Contributor

Choose a reason for hiding this comment

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

So when I implemented #23, I shortened this to only key off of Runs? tasks?. It loosens the general word order. You know need to include this for example. This reintroduces those changes plus some. Let's talk through ways of expressing watch without requiring such a rigid expression.

Copy link
Author

Choose a reason for hiding this comment

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

I can take out the this to be optional

Copy link
Author

Choose a reason for hiding this comment

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

Actually, this now enables several expressions:

Run task `lint` in parallel and watch ...
Run tasks `lint`, `build` and `clean` in parallel and watch ...
Run task `lint` after this / before this in parallel and watch ...
Run task `lint` and watch ...

Copy link
Author

Choose a reason for hiding this comment

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

And I really don't think the user feels more convinient writing

Run task `lint` after in parallel

instead of

Run task `lint` after this in parallel

as latter would be a proper sentence

@zephraph
Copy link
Contributor

zephraph commented Jun 6, 2018

This isn't immediately clear to me...

Run task `lint` before this and watch `*.js` 

What's the actual intent? Watch all js files for changes and when a change happens rerun lint? Does the actual task that the watch command in rerun?

What about complex expressions? Let's say I'm running a command dev. I want it to clean and then lint and build on every js change.

Runs task `clean`
Runs tasks `lint` and `build` when `*.js` files change

Maybe static runs and watch runs shouldn't be in the same sentence. It might be shorter that way, but it's not necessarily any more clear.

Thoughts @flxwu?

@@ -1,6 +1,11 @@
const chokidar = require('chokidar')
const logger = require('./logger')

Array.prototype.asyncForEach = async function(callback) { // eslint-disable-line
Copy link
Contributor

@tunnckoCore tunnckoCore Jun 6, 2018

Choose a reason for hiding this comment

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

Ouch, why? I just really can't believe what i'm looking 🤣

p-map-series or p-map is probably better fit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... I missed that. Yeah, probably don't want to modify the prototype.

Copy link
Author

Choose a reason for hiding this comment

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

okay thanks @olstenlarck

@flxwu
Copy link
Author

flxwu commented Jun 7, 2018

@zephraph

Run task `lint` before this and watch `*.js` 

will run lint on every js file change

Your dev would have to look like that if you want it to do all three tasks clean,lint,build on a JS file change:

Runs tasks `clean`, `lint` and `build` and watch `*.js`

If you want to run clean once, you'd have to do it like that:

Runs task `clean`
Runs tasks `lint` and `build` and watch `*.js`

However, this is not implemented yet I think, or is it? I mean multiline tasks @zephraph @olstenlarck @egoist

@tunnckoCore
Copy link
Contributor

tunnckoCore commented Jun 7, 2018

@flxwu seems like it works, but not correctly.

### baw

Some sample task

Run task `foo` after this
Run task `bar` after this

```bash
echo "baaaawww"
```

### foo

Somedadadadad

```bash
echo "foo"
```

### bar

Sosasasas

```bash
echo "bar"
```

outputs

❯ yarn maid baw
yarn run v1.7.0
$ node bin/cli baw
[15:16:26] Starting 'baw'...
baaaawww
[15:16:26] Finished 'baw' after 15 ms...
[15:16:26] Starting 'foo1'...
foo
[15:16:26] Finished 'foo1' after 6 ms...
[15:16:26] Starting 'bar'...
bar
[15:16:26] Finished 'bar' after 6 ms...
Done in 0.47s.

btw, does this PR support another than this? I mean

Run task `foo` after `bar`

kinda thing?

@flxwu
Copy link
Author

flxwu commented Jun 7, 2018

Run tasks `foo` after `bar`

Does it work in the current version? @olstenlarck

@tunnckoCore
Copy link
Contributor

dont know, didnt thought to try.

@egoist
Copy link
Owner

egoist commented Jun 7, 2018

Definitely should not support Run task `foo` after `bar` 😂

@tunnckoCore
Copy link
Contributor

huh, why not?

@zephraph
Copy link
Contributor

zephraph commented Jun 7, 2018

It's not really needed, is it? #23 loosens the restrictions so Run tasks `foo` after `bar` works, but it might not do what you think. Run tasks triggers the language expression. foo and bar are collected as tasks to run. after means that the commands should run after the current command.

@flxwu
Copy link
Author

flxwu commented Jun 15, 2018

@egoist Are you going to merge this or what is still missing? 😄

@flxwu flxwu mentioned this pull request Aug 13, 2018
@jakepearson
Copy link
Collaborator

@flxwu I have access to start merging, would you fix the conflicts and I will merge today. Thanks.

Felix Wu added 2 commits October 21, 2018 21:49
@flxwu
Copy link
Author

flxwu commented Oct 21, 2018

Done! @jakepearson

@flxwu flxwu closed this Jul 31, 2021
@flxwu flxwu deleted the feature/watch-files branch July 31, 2021 22:29
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.

Watch files
5 participants