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 error: TypeError: i is not a function #4204

Merged

Conversation

fredden
Copy link
Contributor

@fredden fredden commented May 10, 2024

There is a race condition in Alpine when using x-transition and x-show. It's tricky to reproduce. The code change here removes the JavaScript error: Uncaught (in promise) TypeError: i is not a function.

Normally, el._x_hidePromise is a Promise, so calling this is fine. However, when this code runs more than once in quick succession, one copy works just fine, and the next copy passes an undefined into the Promise.all(), so the .then() gets an undefined, which it then attempts to call as a function, resulting in the error reported.

The scenario that I've been looking into is a drop-down menu layout where putting my mouse over a top-level element reveals a sub menu. When I move my pointer over these top-level elements slowly, all is well. When I move my pointer over these top-level elements quickly, all is well. When I move my pointer back-and-forth over these top-level elements quickly, I can trigger this bug.

With the code changes in this pull request applied, I am no longer able to reproduce the bug with these same steps. As far as I can tell, this fixes #1672 and fixes #3832 (comment).

Copy link
Contributor

@ekwoka ekwoka left a comment

Choose a reason for hiding this comment

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

woohoo!!!!

I'ts mostly a non-destructive error, but nice to get it fixed.

@@ -173,7 +173,7 @@ window.Element.prototype._x_toggleAndCascadeWithTransitions = function (el, valu
let carry = Promise.all([
el._x_hidePromise,
...(el._x_hideChildren || []).map(hideAfterChildren),
]).then(([i]) => i())
]).then(([i]) => i && i())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
]).then(([i]) => i && i())
]).then(([i]) => i?.())

Not sure if we want to reopen this can of worms with people using decade old webpack versions that can't support optional chaining, but we can dream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. Support for optional chaining seems to be wide-spread now: https://caniuse.com/mdn-javascript_operators_optional_chaining
And it seems that we're already using this feature in this code-base, so let's use this here.

if (descriptor?.set && descriptor?.get)

let active = data.__context?.getActiveItem()

@fredden fredden requested a review from ekwoka May 11, 2024 09:08
@calebporzio calebporzio merged commit ad13dbf into alpinejs:main May 13, 2024
1 check passed
@calebporzio
Copy link
Collaborator

Thanks!!

@fredden fredden deleted the bugfix/x-transition/i-is-not-a-function branch May 13, 2024 16:14
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.

None yet

3 participants