-
Notifications
You must be signed in to change notification settings - Fork 48
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 sequenceId slot and fix effect promises #201
base: gh-pages
Are you sure you want to change the base?
Conversation
@gterzian, do you want to check this also? |
Will need to check if the original issue is observable in any browser or if there was code already preventing this. Might be good if we can come up with a test to confirm (seems it might be possible to do so). |
@nondebug I'm still not sure what happens if the identifiers don't match... feels like there should be some kind of assert or something somewhere to assuring nothing is getting out of sync. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with a few nits to consider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding @gterzian's suggestions would be great. Can review again after those updates.
@gterzian Thanks for the suggestions! @marcoscaceres Please take another look |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approving, but we should add a test so I can check WebKit.
Closes #200
The following tasks have been completed:
Implementation commitment:
Preview | Diff