sweetalert2-react-content icon indicating copy to clipboard operation
sweetalert2-react-content copied to clipboard

Support context

Open omerman opened this issue 3 years ago • 9 comments

// App.tsx

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

ReactSwal.fire(...) // will work with the context

omerman avatar Aug 03 '22 20:08 omerman

@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

zenflow avatar Aug 03 '22 23:08 zenflow

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 avatar Aug 04 '22 03:08 omerman

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

  • A more elegant solution could be done here to support multiple contexts, and leverage React.Context & hooks.. But I think the current solution can suffice for now.
App = () => 
    <Router ...>
    <ThemeProvider ..>
        {...}
        <SwalRoot />
    </ThemeProvider>
    <Router />
ReactSwal.fire(...) // would have the desired contexts because it uses React's Portal api.

omerman avatar Aug 04 '22 19:08 omerman

@ zenflow 😢 ?

omerman avatar Aug 10 '22 23:08 omerman

@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 avatar Aug 26 '22 11:08 omerman

@limonte

omerman avatar Aug 26 '22 11:08 omerman

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.

zenflow avatar Aug 26 '22 15:08 zenflow

@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 avatar Aug 26 '22 15:08 omerman

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

omerman avatar Aug 26 '22 15:08 omerman