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

Catch the finished promise AbortError and ignores it #259

Merged
merged 13 commits into from
Oct 22, 2024

Conversation

jaywhy
Copy link
Contributor

@jaywhy jaywhy commented Oct 11, 2024

Fixes issue #247 by putting duct tape over it.

@excid3
Copy link
Owner

excid3 commented Oct 15, 2024

Think there's any downsides to doing this?

@jaywhy
Copy link
Contributor Author

jaywhy commented Oct 16, 2024

Yes, I think there could be unforeseen downsides.

Thinking about it more, I wouldn't say I like this patch. It doesn't fix the problem, it just covers it up. If the AbortError is caught, animation glitches still occur. It only fixes the issue with Promise.all rejecting ALL animations when one promise is rejected. So, one animation won't cause others to stop.

I tested el-transition and Alpine.js yesterday. el-transition has the same problem. Alpine.js doesn't, but the Alpine folks do something completely different in their transition library.

I have some time later this week. I can look into it more.

@excid3
Copy link
Owner

excid3 commented Oct 16, 2024

Ours is a fork of el-transition trying to fix a few things, so it's very similar. I tried to understand how Alpine does it but couldn't quite wrap my head around it.

Appreciate your help on this! It's definitely out of my depths.

@jaywhy
Copy link
Contributor Author

jaywhy commented Oct 21, 2024

Ok. I tried to fix it without rewriting it, but I couldn't get that to work.

Are you okay with me rewriting the transition.js library like this?

The code fixes the issues with AbortError's. The transition code now works sort of like Alpine.js. It sets a _stimulus_transitions property on the element, and if another transition comes along and sees that, it will cancel the already running transition.

However, the new transition.js breaks some tests. But I think the broken tests are all timing-related or race condition-like things because the code no longer awaits on the animation Promises. I don't think this will affect production code unless someone relies on these weird timing inconsistencies. Famous last words, though.

I'll give a couple of examples of the test problems...

The dropdown_test.js needs to run await nextFrame() twice, not once, for it to pass. It requires two calls because the transition.js calls requestAnimationFrame twice and doesn't use await.

One of the alert_test.js tests breaks because the closeButton.click() event in that test now happens before the setTimeout() in AlertController#connect() gets to run. The click() calls leave(), and then the setTimeout() code runs, calling enter(), which cancels the leave().

If you're ok with this change, I'll clean things up, fix the broken tests, and squash my commits.

@excid3
Copy link
Owner

excid3 commented Oct 21, 2024

This is looking good @jaywhy! Will give it a test locally later today, but it seems good to me.

@excid3
Copy link
Owner

excid3 commented Oct 22, 2024

Looks like it's working great!

Anything else to do or is it ready to merge?

@jaywhy
Copy link
Contributor Author

jaywhy commented Oct 22, 2024

Yes. I think so.

@excid3 excid3 merged commit daaf703 into excid3:main Oct 22, 2024
1 check passed
@excid3
Copy link
Owner

excid3 commented Oct 22, 2024

Thanks so much for this @jaywhy 👍

@jaywhy jaywhy deleted the fix-abort-error branch October 22, 2024 15:15
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.

Javascript Error on Popover: Uncaught (in promise) DOMException: The user aborted a request
2 participants