Skip to content

Add option to run dependencies asynchronously#106

Merged
joerdav merged 9 commits into
joerdav:mainfrom
stephenafamo:async-deps
Dec 4, 2023
Merged

Add option to run dependencies asynchronously#106
joerdav merged 9 commits into
joerdav:mainfrom
stephenafamo:async-deps

Conversation

@stephenafamo

@stephenafamo stephenafamo commented Nov 28, 2023

Copy link
Copy Markdown
Contributor

Fixes #105

@joerdav

joerdav commented Nov 29, 2023

Copy link
Copy Markdown
Owner

This looks great! Thanks for the contribution. Would you kindly add some documentation on this feature before I accept the PR https://github.com/joerdav/xc/tree/main/doc/content/task-syntax

Thanks!

@stephenafamo

Copy link
Copy Markdown
Contributor Author

Documented 👌🏾

@joerdav

joerdav commented Nov 29, 2023

Copy link
Copy Markdown
Owner

Brilliant, thanks for that.

Now that parrallel tasks are possible do you think we should do something about the STDOUT of the tasks? Since they may get mangled now.

And a question on runDepsAsync currently we're using an errgroup, so it'll exit as soon as there is an error, do you think it would make more sense to await the other deps to conclude before ending?

@stephenafamo

Copy link
Copy Markdown
Contributor Author

Now that parrallel tasks are possible do you think we should do something about the STDOUT of the tasks? Since they may get mangled now.

Well.... the ideal experience may be to do like docker-compose and prefix each log line with the task name. But that adds a bit more complexity.

And a question on runDepsAsync currently we're using an errgroup, so it'll exit as soon as there is an error, do you think it would make more sense to await the other deps to conclude before ending?

Well, I could see people wanting both ways, but I honestly don't know which would be better. I almost want to suggest have both behaviours as options, but it starts to feel even more complex.

The other idea I had was to allow the user group the async task with brackets. Something like this.

requires: build-assets, [watch-assets, watch-server], cleanup

Which will run the build assets first, then run the watchers async, before the cleanup. But again... more complexity.

@joerdav

joerdav commented Nov 30, 2023

Copy link
Copy Markdown
Owner

I think we can maybe tackle the logging of async tasks later.

For me I think the sensible default would be for the async tasks to all complete before continuing. I think after that change I would be happy to accept the change provided all the workflows pass!

I believe the grouped behaviour is currently achievable once we merge this branch too (even if it's a bit messy):

## run

requires: build-assets, watch, cleanup

## watch

requires: watch-assets, watch-server

runDeps: async

## watch-server
...

## watch-assets
...

## build-assets
...

## cleanup
...

@stephenafamo

Copy link
Copy Markdown
Contributor Author

Made the change to not exit on the first error.

@stephenafamo

Copy link
Copy Markdown
Contributor Author

I've added log prefixes to all scripts run by xc. And if the task has dependencies, it will pad the prefixes so it all looks nice.

I understand that this would technically be a breaking change, but I think it is allowed since xc is still in v0

@stephenafamo

Copy link
Copy Markdown
Contributor Author

I've also fixed a bug with xc not recognizing attributes on the last line of the file in 5b9ae35

I'm not sure if that's the best solution, so please take a look

@joerdav

joerdav commented Dec 1, 2023

Copy link
Copy Markdown
Owner

It looks like the lint step failed, are you able to fix these? It looks like there was also a large drop in test coverage, I think some tests around the log file you created should be acceptable.

@stephenafamo

Copy link
Copy Markdown
Contributor Author

It looks like the lint step failed, are you able to fix these? It looks like there was also a large drop in test coverage, I think some tests around the log file you created should be acceptable.

Fixed linting and added tests

@stephenafamo

Copy link
Copy Markdown
Contributor Author

Can you take another look @joerdav

@stephenafamo

Copy link
Copy Markdown
Contributor Author

I've also added an "Interactive" attribute with some documentation of where it can be useful.

@joerdav joerdav merged commit 01aa6e0 into joerdav:main Dec 4, 2023
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.

Running task dependencies async

2 participants