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

fix: process nested sequence messages in order #843

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Hrily
Copy link

@Hrily Hrily commented Oct 15, 2023

Fixes: #680

@Hrily
Copy link
Author

Hrily commented Oct 15, 2023

hrily/bubbletea/examples/nested-sequence is the example to reproduce the bug from the issue and gives the expected output with the fix from this commit:



    Searching for something...
  ──────────────────────────────




  Something is not loaded!
  Something is loaded!

// process nested sequence messages in order
if sequenceMsg, ok := msg.(sequenceMsg); ok {
for _, cmd := range sequenceMsg {
p.Send(cmd())

Choose a reason for hiding this comment

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

What if we have deeply nested sequences (3+)? Imagine we have a cmd like:

sequenceMsg: [
    sequenceMsg: [
        sequenceMsg: [customMsg11, customMsg12],
        sequenceMsg: [customMsg21, customMsg22]
    ]
]

where each Msg represents a cmd and what will be returned after running it.

Doesn't this solution only work for the first two sequences? The outermost sequence is handled by line 363, and then the second sequence is handled here, but the third and fourth sequences (with the customMsg*s) will be done concurrently -- customMsg11 will happen before customMsg12, but customMsg21 could occur before customMsg11, after customMsg11, or after customMsg12. Am I missing something?

@aymanbagabas aymanbagabas deleted the branch charmbracelet:main October 28, 2024 17:41
@aymanbagabas aymanbagabas reopened this Oct 28, 2024
@aymanbagabas aymanbagabas changed the base branch from master to main October 28, 2024 17:55
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.

Incorrect order when nesting Sequence commands
3 participants