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

Doesn't work when there is nested portals.OutPoral #32

Open
loia5tqd001 opened this issue Nov 10, 2021 · 6 comments
Open

Doesn't work when there is nested portals.OutPoral #32

loia5tqd001 opened this issue Nov 10, 2021 · 6 comments

Comments

@loia5tqd001
Copy link

Here is my code

<div>
  <portals.InPortal node={colorPortalNode}>
    <ColorProvider />
  </portals.InPortal>
  <portals.InPortal node={counterPortalNode}>
    <CounterProvider />
  </portals.InPortal>
  <portals.OutPortal node={colorPortalNode}>
    <portals.OutPortal node={counterPortalNode}>
      <button onClick={() => setPage((prev) => (prev + 1) % pages.length)}>
        Change Page
      </button>
      {pages[page] === "counter" && <Counter />}
      {pages[page] === "color" && <Color />}
    </portals.OutPortal>
  </portals.OutPortal>
</div> 

It seems only the inner portals.OutPortal takes effect whereas the outer doesn't.
Aka:

  • Expect: Children can consume both CounterProvider and ColorProvider
  • Actual: Children can consume only CounterProvider

Here is the codesandbox of the problem

@pimterry
Copy link
Member

Woah, ok. Even passing children to OutPortal is pretty unusual, let alone trying to nest that! I can see how this might be useful for these kinds of provider use cases, but it's difficult to think through exactly how this should work. I'm not aware of any good reason why this wouldn't work, but I've never tested it before myself.

That said, I'd be happy to support this if it's possible to do so without adding too much extra complexity or breaking anything else.

Do you want to look into it? If you can just work out why it doesn't work already then that'd be very useful, and if you want to open a PR to fix it that'd be even better.

@loia5tqd001
Copy link
Author

Thanks for so quick reply @pimterry , actually before opening this issue I've tried to understand the code myself already, but there was no luck cause I'm still a noob lol. However let me try for several extra hours, if I still can't figure it out, perhaps I will need your need. Thanks.

@loia5tqd001
Copy link
Author

Hmm it seems because the original design didn't expect OutPortal to have children in mind from the first place (as you stated above).

Instead of

App
  InPortal
    Constate
      useColor.Provider
  InPortal
    Constate
      useCounter.Provider
  OutPortal // for InPortal>Constate>useColor.Provider
    div
      OutPortal // for InPortal>Constate>useCounter.Provider
        div
          button
          Counter
            ...

here it is the actual component tree
image

so the behavior instead of replacing OutPortal with InPortal, it brings all props (including children) from OutPortal to InPortal.


Just an update to you, I'll diagnose closer the code.

@pimterry
Copy link
Member

Hi @loia5tqd001 did you make any more progress here? Let me know if there's anything I can do to help.

@loia5tqd001
Copy link
Author

Hi @pimterry I really have no idea how to tweak the package to make it work for this use case 🤥 like I don't even know which question to ask, otherwise I asked already

@pimterry
Copy link
Member

Ok, fair enough. Yes, this use case isn't a priority for me right now, or for normal usage in general I think, but I would like to support it in future if possible. I don't think there's anything that would make this impossible, it's just a bit difficult to work out the details.

If you or anybody else does work out the underlying issue here and put a PR together to fix it, I'd be very happy to look at that. I'll leave this open for now.

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

No branches or pull requests

2 participants