这是indexloc提供的服务，不要输入任何密码
Skip to content

Run setup tasks synchronously #95

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 26, 2023
Merged

Run setup tasks synchronously #95

merged 2 commits into from
Nov 26, 2023

Conversation

@thisistonydang
Copy link
Contributor

@thisistonydang thisistonydang commented Nov 24, 2023

Summary

Change LiveSvelte setup tasks to run synchronously.

Reason for change

Currently the setup tasks run asynchronously. However this could lead to jumbled terminal output such as in the following screenshot:

Screenshot 2023-11-23 at 16 51 57

This is not ideal because the user installing live_svelte may miss the prompt for the file overwrite and think that the process just hung. (This is what happened to me and it took a while to figure out what was wrong 😅). Changing the setup tasks to run synchronously will show a clearer output to the user:

Screenshot 2023-11-23 at 17 02 18

@woutdp
Copy link
Owner

woutdp commented Nov 24, 2023

Nice!

Did you look into using Stream.map for this instead? The previous code is slightly more readable but I do like your solution more as it solves a real issue. We might be able to keep the previous code with your fix by using Stream.map. But I'm unsure if that would work, would have to test it myself but since you're already working on it might be faster if you check. If it doesn't work then feel free to just say so and I'll merge :)

@thisistonydang
Copy link
Contributor Author

thisistonydang commented Nov 24, 2023

I haven't looked at using Stream.map yet, but I can definitely take a look. I'm out the rest of this weekend, but will try it out first chance I get and let you know!

thisistonydang
thisistonydang commented Nov 26, 2023
View reviewed changes
lib/mix/tasks/setup.ex
]
|> Enum.map(&Task.async(fn -> Mix.Task.run("live_svelte." <> &1) end))
|> Enum.map(&Task.await(&1, :infinity))
|> Enum.each(&Mix.Task.run("live_svelte." <> &1))
Copy link
Contributor Author

@thisistonydang thisistonydang Nov 26, 2023

Choose a reason for hiding this comment

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

Note: Enum.each is used over Enum.map because running the tasks are side-effects and we don't need anything to be returned here.

@thisistonydang
Copy link
Contributor Author

thisistonydang commented Nov 26, 2023

I took another look and realized I was overcomplicating things. The Enum module functions already runs things synchronously so there was no need to use Task.async and Task.await. I refactored the code to be a simple and clear one-liner using Enum.each. Please see the latest commit and lmk if things look better now!

@woutdp
Copy link
Owner

woutdp commented Nov 26, 2023

Nice, even better!

@woutdp woutdp merged commit f76ace0 into woutdp:master Nov 26, 2023
@thisistonydang thisistonydang deleted the tonydangblog/sync-setup branch November 27, 2023 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@thisistonydang @woutdp