Proposal of passing position property directly to alerts instead of render multiple Providers

[besLisbeth]

Hi @schiehll!

I have an issue with using a new multiple providers feature. In my current project, I have a need to render alerts in three different positions. So, my main HOC looks the next way:

const BottomRightAlertContext = createContext();
const BottomLeftAlertContext = createContext();

const NotificationProvider = withRouter(({ children }) => {
    const alertOptions = {
        offset: '25px',
        timeout: 3000,
        transition: 'scale',
        template: Alert
    };

    return (
        <AlertProvider
            position={positions.BOTTOM_RIGHT}
            context={BottomRightAlertContext}
            {...alertOptions}
        >
            <BottomRightAlertContext.Consumer>
                {bottomRight => (
                    <AlertProvider
                        position={positions.BOTTOM_LEFT}
                        context={BottomLeftAlertContext}
                        {...alertOptions}
                    >
                        <BottomLeftAlertContext.Consumer>
                            {bottomLeft => (
                                <NotificationConsumer alerts={{ bottomRight, bottomLeft }}>
                                    {children}
                                </NotificationConsumer>)
                            }
                        </BottomLeftAlertContext.Consumer>
                    </AlertProvider>)}
            </BottomRightAlertContext.Consumer>
        </AlertProvider>
    )
});
export default NotificationProvider;

Why do I pass prop alerts to NotificationConsumer which I get from Context.Consumer - I use MobX and usage of its high-order functions (etc inject, observer) limiting me in using hooks in components (I can use them in components without MobX decorators or HOF). If to talk about withAlert HOC - it's clear, that I can't get both references to the bottomRight and the bottomLeft alert at the same time.

Also, I did not add AlertProvider with the third needed position because of making the code look extremely ugly.

My proposal is to pass property position to Provider as it used now and also add property position directly to the alert which will overwrite the Provider position. In that case if user needs to show alerts in only one position it won't be the break changes, but for multiple positions - it will become less complicated. I think there can be used an approach which I used in PR #91.

I hope that you have an idea of how to help in my situation. If you need a PoC of the idea above - I can open new PR.

[schiehll]

Hey @besLisbeth!

I think I have a solution, but it involves installing a new dependency in your project: this one. It has only 1kb tho, so I think it's ok.

Here's a working example: https://codesandbox.io/s/k99o44y40o

Let me know if it helps!

[besLisbeth]

Thank you a lot for showing me the package, but the question was not itself in using hooks. What I didn't like in with-hooks-support - it added the possibility to use hooks in Component's render method. It means that render stop being pure and it seems to me improper for React philosophy.

I just proposed to avoid Provider's nesting.

[besLisbeth]

I think that it's not overwhelming to add property position to alert instead of rendering multiple Providers. Besides, it can be an additional feature.

I added that in my local branch. Maybe you would like to have a look?

[schiehll]

Yeah, I know that the question was actually how to use more than one alert consumer, which is possible with the HOC, but way easier with hooks, so that's why I suggested you that solution.

Here's one way to solve it using only the withAlert HOC: https://codesandbox.io/s/o59mq0v3qz

About the nested provider thing, are you having any real performance issue with this approach? If that's the case we can try to find alternatives.

I would prefer not to have to include more loops and logic (hurting performance a little) for all use cases only to solve an edge one.

[besLisbeth]

Thank you for the feedback. About performance issue - from my point of view multiplying the Providers and Consumers is a more inefficient way than having only one Provider and one but flexible Consumer.

I've got two flame charts - one using master branch - with two Providers, and second - from my branch with one Provider but using local alert position.

master branch

my branch

There you can see, that having multiple Providers is a more consuming way than render multiple Wrappers.

Also, if you'll look at tests in my branch, you'll see that I didn't modify any of them - I just added new tests for the new functionality, so I propose that it is fully compatible with the existing features.

[schiehll]

Nice! That's a considerable improvement. But can you do the same comparison using only one position with both, your branch and the master one? I just wanna make sure that it doesn't hurt the most common use case too much.

[besLisbeth]

Here you are - almost the same

master branch

my

[schiehll]

Awesome!

Can you open a PR and maybe update the docs?

I will have a busy day today, but later I will try to merge and publish it.

Also, I don't know how I let it pass, but you could just use the Context.Consumer instead of all that withAlert juggle I did in the second example haha.

Anyway, will wait for your PR! Thank you!

[besLisbeth]

Thank you, Reinaldo! 🥳