solidjs-community / solid-motionone

A tiny, performant animation library for SolidJS
MIT License
112 stars 3 forks source link

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

Closed trival closed 1 month ago

trival commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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...

thetarnav commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

thetarnav commented 1 month ago

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 commented 1 month ago

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