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

Support context #207

Open
wants to merge 229 commits into
base: main
Choose a base branch
from
Open

Support context #207

wants to merge 229 commits into from

Conversation

omerman
Copy link

@omerman omerman commented Aug 3, 2022

// App.tsx

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

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

zenflow and others added 30 commits March 2, 2018 14:53
<a name="0.1.6"></a>
## [0.1.6](sweetalert2/sweetalert2-react-content@v0.1.5...v0.1.6) (2018-03-05)

### Bug Fixes

* **ci:** Upgrade `semantic-release` and `[@semantic-release](https://github.com/semantic-release)/git` packages ([bc2862a](sweetalert2@bc2862a))
* chore(package): update dependencies

* chore(package): Update more dependencies

* chore(package): Downgrade webpack-serve back down to v0.1.5 and add to package.greenkeeper.ignore
@zenflow
Copy link
Member

zenflow commented Aug 3, 2022

@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
Copy link
Author

omerman commented Aug 4, 2022

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
Copy link
Author

omerman commented Aug 4, 2022

@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
Copy link
Author

omerman commented Aug 10, 2022

@ zenflow 😢 ?

@omerman
Copy link
Author

omerman commented Aug 26, 2022

@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
Copy link
Author

omerman commented Aug 26, 2022

@limonte

@zenflow
Copy link
Member

zenflow commented Aug 26, 2022

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
Copy link
Author

omerman commented Aug 26, 2022

@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
Copy link
Author

omerman commented Aug 26, 2022

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

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

Successfully merging this pull request may close these issues.

7 participants