sweetalert2 / sweetalert2-react-content

Official SweetAlert2 enhancer adding support for React elements as content
MIT License
695 stars 47 forks source link

Support context #207

Open omerman opened 2 years ago

omerman commented 2 years ago
// App.tsx

<MyProviders>
   <SwalRoot /> // This is where the magic begins
</MyProviders>
//Somefile.ts

ReactSwal.fire(...) // will work with the context
zenflow commented 2 years ago

@omerman Thank you for your interest in solving this issue (#192)!

I don't want to merge this however because of some limitations it has:

  1. Only supports one context provider
  2. Doesn't support passing any props to MyContextProviderComponent (e.g. MyContext.Provider value, or HistoryRouter router). This is severely limiting.
  3. Doesn't help in these scenarios, where (even if # 2 is fixed) you're unable to get the correct context value:
    1. when the context value is passed as a prop to the context provider from a component's state (example: https://reactjs.org/docs/context.html#updating-context-from-a-nested-component)
    2. when the context is provided by a framework that calls your code, e.g. context used for Next.js's useRouter

Also, not exactly a limitation but an ergonomic thing: you would have to explicitly specify which context(s) you want to provide, when I think we can assume users just want whatever contexts are provided to the component from which the alert is fired.

I believe a more complete solution is possible, even one that covers # 3 and the ergonomic issue. I believe the more complete solution would involve using portals to render the given react element(s) within the component from which the alert is fired. Portals are needed to do this since the alert is rendered (without React) outside your React app's DOM tree

omerman commented 2 years ago

At first glance I tried seeking a solution that will use the App context, such as portals, but there wasn't a quick one to write. The solution I propose simply gets rid of the boilerplate of rendering a context for each usage. I will take another crack at it, I think I have a more elegant idea on how to impl this with a poral. I'll keep you posted 😊

omerman commented 2 years ago

@zenflow I've added support for Portal. The code itself is not ideal, but it works as expected. It should also solves some of the issues you mentioned.

App = () => 
    <Router ...>
    <ThemeProvider ..>
        {...}
        <SwalRoot />
    </ThemeProvider>
    <Router />
ReactSwal.fire(...) // would have the desired contexts because it uses React's Portal api.
omerman commented 2 years ago

@ zenflow 😢 ?

omerman commented 2 years ago

@zenflow In the meantime I'm using my fork, but I really think you should consider approving this pr. I changed it to use Portals as we discussed. It works well, Im using it for including my Mui Theme provider for example..

Please let me know what you think

omerman commented 2 years ago

@limonte

zenflow commented 2 years ago

Hi @omerman sorry for the long delay. I'll have a look at this on Sunday. Just two notes for now:

  1. This will need tests. I actually did a bit of work on this issue a few months ago and started with a test which I think would now pass with your changes, so you can just hold tight on this one
  2. I have a bit of concern about the lack of support for multiple contexts. I am wondering:
    • what will happen if someone tries to use multiple contexts? The problem might not be obvious. Could we somehow throw an error?
    • will it be possible to add that support in the future without confusing breaking changes? Answering this would take some consideration into how it would work and what the API would look like. If we're able to answer that then we already basically have the support lol. This is a big additional challenge though, and since your implementation satisfies the common use cases, it might suffice. Though if we don't have support for multiple contexts from the beginning, it will be more challenging to add it later, for both maintainers/contributors and users.
omerman commented 2 years ago

@zenflow I think that 99% of the usecase will not need to set up different Swal behaviors, meaning it will probably be placed under the app providers and be expected to work under those contexts.

I did fiddle around with the concept of placing a Swal provider under different tree levels of react dom, exposing hooks and holding a token that will be used when Launching a popup, but I decided it's a small edge case.

Having said that, If we decide to support this case, I believe it can be done without breaking the current behavior. It could have the same api and if not supplied with a Swal hook's key while launcing a pop up, we will take the closest to Root node Swal context.

Hope you understood my explanation 😅

omerman commented 2 years ago

And regarding your tests, If you want I'll add you as a collaborator on my fork and you can add the files, or send it here as a comment and I'll run it