-
Notifications
You must be signed in to change notification settings - Fork 106
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
Create Tunnel #168
Create Tunnel #168
Conversation
…r is likely caused by a bug in React. Please file an issue."
…stance<T>` - seems like createPortal is adding to root container. Something wonky going on here. I may need to attach everything to the babylon object (and return directly as public instance) and that has some odd side-effects for swapping children/fibers etc.
…er.ts and in storybook. portal is now outside of transformnode -> box is moved from outside into node
…into create-portal
…io would loose refs in some cases
… 0 on first render, led to vec being [NaN, NaN], therefore just added an isNaN check.
deleted the wrong branch...woops - reopening and there is still an issue with removing elements from array. I need to have unique identifiers within the store to know which element to add and remove. |
…emoving of components. improvement: filtering of tunnels at exit
…d operator on remove
Thanks @dennemark for this great PR!! I am hesitant to add zustand as a dependency for a single component. If I were to replace the current hooks with a better state management that didn't suffer from current issues with reconciler context boundaries then for sure would bring in a library like zustand. In the meantime I would like to move zustand to storybook and add this as an integration story, but certainly really like the idea. Just trying to keep bundle size down unless it's part of core functionality. Besides reconciler there are currently no dependencies outside babylonjs and the upcoming v4 will remove Do you mind if I accept and then move to storybook for now? Keen to have a discussion to move the hooks like |
@brianzinn this makes sense! Sure it is possible to move it all to storybook! Actually the tunnel component might be useful for other libraries, too, like r3f, pixi, kanva. As far as I could see, they face similar issues with renderers. So it might be useful to use this as a separate npm. Would be my first npm package, lets see. We could see how much the other hooks would profit from a different state library like zustand. If we decide to add it as a new dependency, we could improve the html component, too. I did it in my project today and now I can style my components with context of the renderer surrounding react-babylonjs. Its pretty convenient now. Hope no sideeffects will happen, i.e. too inefficient rerendering of children. |
I think zustand selector prevents unnecessary rerenders:
|
if it's OK with you I will merge this PR next week and move some pieces to storybook. it's a really good recipe 😄 |
sure sounds good! |
@dennemark @brianzinn i think it can still be simplified without having to rely on id's. i've made a small library on poimandres to test it out, modelled it after React.context: https://github.com/pmndrs/tunnel-rat now tunneling through multiple renderers can still have unforseen side effects, useEffect practically looses all guarantees to catch a ref next, especially with suspense and concurrency. that i think cannot be fixed in userland. |
nice! great to see another view on this from a more experienced developer :) good learning material and cleanup! i had a version without id´s, too. but its disadvantage is, that one can only have one tunnel entrance and exit. with ids, we can create multiple tunnel exits and entrances from same component. in my case i used one exit within my |
multiple exits are possible, <dom.Out />
<div>
<dom.Out />
<dom.Out />
</div> multiple ins without GUID perhaps as well but this would seem a little too crazy given that there's no sane order after this, while multiple outs can at least be composed. as for uLE, it fires before the view is committed, i think it could be a good place for it. |
@dennemark - we should bring in Zustand as a dep. can get to that after this current work on docs. i have a few other breaking changes i want to bring in, so good chance for me to do a good cleanup. |
you have a better overview, but is it still possible to avoid zustand in react-babylonjs and create a seperate package containing the additional components using it? - maybe we should talk about it in discussion :D |
it is entirely possible to avoid zustand as status quo - would be nice to bring it in though as a way to not have to bridge across renderer boundaries and cleanup the context provider code. i'll bring up a discussion once I get through the big list of todos! 😄 |
hi @brianzinn,
I figured out a way to pass components from one renderer to another while keeping context and refs.
Since portal stay within renderer and bridges only pass context hierarchically into other renderers, i justed named it tunnel.
I added some comments to the component:
I am not sure, how stable it is.
Besides the storybook, which sends bjs components from outside engine into engine, i also tried to send reactdom data from within bjs outside of it. it connects to the context of the renderer it is send to. and refs seem to work, too.
i used zustand to make this work. also tried valtio, but had issues with rerendering and keeping refs. both state libraries can live outside of react, but trigger state changes. therefore they seemed ideal to send data from a to b. hope it will be stable..