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

Add option to run dependencies asynchronously #106

Merged
merged 9 commits into from
Dec 4, 2023

Conversation

stephenafamo
Copy link
Contributor

@stephenafamo stephenafamo commented Nov 28, 2023

Fixes #105

@joerdav
Copy link
Owner

joerdav commented Nov 29, 2023

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
Contributor Author

Documented 👌🏾

@joerdav
Copy link
Owner

joerdav commented Nov 29, 2023

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
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
Copy link
Owner

joerdav commented Nov 30, 2023

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
Contributor Author

Made the change to not exit on the first error.

@stephenafamo
Copy link
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
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
Copy link
Owner

joerdav commented Dec 1, 2023

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
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
Contributor Author

Can you take another look @joerdav

@stephenafamo
Copy link
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