Always include WithABProvider in islands

[oliverlloyd]

This PR introduces a pattern where we're asking developers to wrap their client side code in WithABProvider. But should we abstract this requirement and simply always set it on every island?

Please give feedback by:
👍 for setting WithABProvider in Island - always on
👎 for keeping WithABProvider outside - off by default

What is WithABProvider?

This is a wrapper component that sets the ABProvider context so that child components can then access the useAB custom hook without errors. This hook expects the context which is provided by ABProvider to exist.

For 👍

Simpler, we don't need developers to think about this and it just works

Against 👎

Performance, we setting lots of context state and adding more code than might be used

[chrislomaxjones]

I’d vote off by default. Somewhat for the reason of performance, but for me it more boils down to a separation of concerns:

• Islands are responsible for the hydration of components client-side.
• WithABProvider is responsible for providing the context required by the useAB hook further down the component tree.

These feel like orthogonal concerns, with their relatedness a consequence of requiring client-side data from the AB test framework.

Of course, Island has to be the parent of WithABProvider, but that feels to me like it should be something you should opt into when necessary, rather than get by default. How many AB tests are running on DCR at any given time, and what fraction of the islands will need to actually know about them?

we don't need developers to think about this and it just works

This is a good point. Given when developing in this area you come up against concepts to do with hydration and AB testing, it's important this is easy and intuitive. I think having a reference implementation in the code, possibly pointed to from documentation, helps with this. Basically a simple example that serves as a recipe for future developers (The Cypress smoke test?) looking to setup a component that uses useAB.

[mxdvl]

If an Island can only have a single child, it seems like we’re breaking React Composition by forcing this one children to wrap itself in a Provider. I think there should be a standard way to do this.

As it stands, in order for a component in an Island to be able to useAB, we need to pass down all the props required for a test to run, which are all available globally–abTestSwitches, mvtId, isDev, isSensitive–which seems a little odd, as we’re duplicating switches information many times–5 as of this writing.

There might be alternatives to WithABProvider altogether:

• Leverage useSWR to get this information once
• Provide a way to get AB data outside of hooks, leaning into the Observable pattern

[mxdvl]

One proposal can be found in #4200.

[jamesgorrie]

We're closing this discussion as stale. Please reopen this if you want to try gain traction on this discussion.