Skip to content

Prevent Presence component to crash if mountedStates does not contain the provided element #11

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

Closed
wants to merge 1 commit into from

Conversation

trival
Copy link

@trival trival commented Sep 19, 2024

I had the situation where mountedStates.get(el) was actually undefined. That throws and breaks the app since the provided guard is not sufficient to prevent the access to undefined.exit.

Please consider adding the extra optional chaining operator to prevent such crashes.

@thetarnav
Copy link
Member

Could you replicate the situation where that happens?
I don’t want to fix an error that I don’t have tests for or even any proof of existing.

@trival
Copy link
Author

trival commented Sep 19, 2024

well, the code probably tries to protect against the situation when mountedStates.get(el) is not defined, but the implemented protection is incorrect, because if that ever happens, the code tries to access undefined.exit and throws. So this change at least improves the quality and robustness of the code.

Anyways, the situation where that actually happens is hard to track. For me it happens when a specific route in solid-router tries to render a <Presence exitBeforeEnter > component. Somehow the onExit callback is fired before the actual element is mounted to the DOM. I am still trying to find another workaround or to track why this is happening, but a single ? would just rescue me from struggles. When i patched this library with the correct chaining operator, everything works again...

trival added a commit to trivial-space/website that referenced this pull request Sep 19, 2024
@thetarnav
Copy link
Member

well, the code probably tries to protect against the situation when mountedStates.get(el) is not defined, but the implemented protection is incorrect, because if that ever happens, the code tries to access undefined.exit and throws

I don't think that's true. .exit won't be accessed if the optional chaining is "broken". It will just take the "else" branch without throwing.

image

Although I don't doubt that there might be some bugs visible when using router with this library, as it means that suspense boundaries and transitions are present. Which may cause the "Somehow the onExit callback is fired before the actual element is mounted to the DOM" situation.

So if you patch it and that works, that's great, but I would still love to find out a reason why that is.
Can you share more info about your app? Especially details around ssr, the routes with presence are setup could help.

@trival
Copy link
Author

trival commented Sep 23, 2024

Thanks for the feedback.

The production code of the library has a different shape, like in solid-motionone/dist/index.js, line 23:

(mountedStates.get(el)?.getOptions()).exit ? el.addEventListener("motioncomplete", done) : done();

The parens, that where inserted in order to make the type cast, are still present in the transpiled code, and that is what throws:

(undefined?.getOptions()).exit ? "1": "0" // ==> Uncaught TypeError: undefined has no properties

The code that throws in my case is a <Show> component, that has a Motion component both as a child as well as a fallback.

The situation that throws is when I initially load the page with a route where the active case of the Show is visible, but i think the onExit callback is called with the fallback component... At the point the error happens, the app is not yet mounted to the dom, the parent container dom node is yet empty.

here is the component that throws: https://github.com/trivial-space/website/blob/3409bee4956dc0532d92f3a96820421d75043028/src/Work.tsx#L151

I hope this helps. Unfortunately I am not that firm with the internals of motion one or solid js to be able to create a minified example in short time...

@thetarnav
Copy link
Member

it's the parenthesis used for typecasting...
I see now

@thetarnav
Copy link
Member

your code looks fine so I'm not sure why onExit would be called at a wrong time
all the setIsPlaying happen in an effect, after the app is loaded

thetarnav added a commit that referenced this pull request Sep 23, 2024
This should remove the parenthesis used for typecasting,
that was causing an issue reported in #11
@thetarnav
Copy link
Member

Will close this in favor of #12 which eliminates the need for typecasting
@trival can you double-check if that works fine?

@thetarnav thetarnav closed this Sep 23, 2024
@trival trival deleted the patch-1 branch September 23, 2024 15:41
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.

2 participants