feat 2130 - ability to add PageBuilder Content Type Configs dynamically

[vchirkov]

Description

PageBuilder/config.js provides two methods: getContentTypeConfig(contentType) and setContentTypeConfig(contentType, config) instead of only getter. This allows to add PageBuilder Content Type Configs dynamically.

When you need to add new content type, you just call setter with proper params

import React from 'react';
import {setContentTypeConfig} from '@magento/venia-ui/lib/components/RichContent/PageBuilder/config';

// something you are doing, that leads to necessity of adding new content type for PB

setContentTypeConfig('Example', {
            configAggregator: (node, props) => {
                return {
                    text: 'example-text'
                };
            },
            component: function ExampleComponent({text}){
                   return <div>{text}</div>;
            }
});

And then call PageBuilder in the place you expect it to be rendered:

import React from 'react';
import PageBuilder from '@magento/venia-ui/lib/components/RichContent/PageBuilder/pagebuilder';

export function PageBuilderComponent({content}) {
    return (
        <div className="page-builder-component">
            <PageBuilder masterFormat={content}/>
        </div>
    );
}

Related Issue

Closes #2130.

Acceptance

Verification Stakeholders

Specification

Verification Steps

two new tests added to config.spec.js.

Screenshots / Screen Captures (if appropriate)

Checklist

• I have added tests to cover my changes.

[PWAStudioBot]

	Messages
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).

Generated by 🚫 dangerJS against 1032546

[zetlen]

This is a great idea. Can you add an example to the pull request description of how you would call this new API from a third-party system?

[vchirkov]

@zetlen, done

[vchirkov]

@sirugh, @davemacaulay, @jimbo, @zetlen Hello guys, is there a chance you could review this MR and leave your comments about what should be improved or help it move on to the next steps with merging in?
Thanks in advance!

[davemacaulay]

@vchirkov thanks for the contribution and making Page Builder better in PWA Studio!

I'm a big fan of all core aspects using the API we provide. What do you think in regard of making all of the existing core content types use the same API as you're providing here?

Doing this leads me to also ask at what point should we call setContentTypeConfig? Do we intend to call this at run time, if so when should this be called and how? or is this something for the build process, if so how do we persist a static config to the application to be consumed at run time?

[vchirkov]

@davemacaulay,

What do you think in regard of making all of the existing core content types use the same API as you're providing here?

Can be done, but I don't see the big benefit for this. Anyway, original set of components is required for normal functioning of PageBuilder itself and will always be available. So I would call initial set "Default Config" and use setContentTypeConfig as a simple mechanism of extending and altering the default one.

when should this be called and how?

Our intention is to use it at run time, when based on some conditions we add or mutate default config with new aggregators and components. We are going to make a custom Provider component, that is going to handle this logic on application start.
As default config is created instantly on first module require, developer doesn't need to worry about the initialization sequence like he would have to in case of dynamic adding default Page builder entities.

This solution closes all our current limitations, is relatively simple and extendable in future. Interface is simple but incapsulates implementation and can be reimplemented in any required way under the hood (if changing current approach would be required).

What do you think?

[davemacaulay]

@vchirkov I agree with the default config part. However, I'm also a strong advocate of using an API you implement within the core product, otherwise it's harder for others to determine how to use this API within code. Though I'm not passionate about it to require the change here as I think having a default config does make sense.

Apart from that sounds good to me, I'll want @zetlen to comment before we move ahead with this!

Thanks again for your hard work on this.

[zetlen]

Our extensibility plan prefers build-time resolution to runtime. We're not completely opposed to a runtime extension point like setContentTypeConfig, though. We know that some use case may come along where we need to run extension dispatch on the user's device. In our own backlog, that use case hasn't come along yet. And for your needs, I can't tell from your explanation why it needs to be at runtime, instead of an extension point that inserts additional type configs at build.

Our intention is to use it at run time, when based on some conditions we add or mutate default config with new aggregators and components. We are going to make a custom Provider component, that is going to handle this logic on application start.
As default config is created instantly on first module require, developer doesn't need to worry about the initialization sequence like he would have to in case of dynamic adding default Page builder entities.

Would the "some conditions" vary based on the logged-in user or other data in the HTTP request? If so, then maybe that's a use case for setContentTypeConfig. If not, then I'd like to explore how we could inline your additional content types into the build by exposing a "target" (like in #2137) that modifies PageBuilder asset code.

That's the clarification I need from you, @vchirkov. In general, I'm very grateful to you for working on framework-level extensibility concepts. Please don't stop! And as for this PR itself: If this suits your needs, I want you to use it. I don't think it conflicts with #2137 as code, but as an approach, it might need to change to be merged into our PageBuilder module.

[vchirkov]

Hi @zetlen, thanks a lot for such detailed description. Now I understand your position much better and did a deeper investigation into BuildBus concept.
I'll try to explain more concretely, what we are trying to solve and hope together we will find the best approach, that will both work for us and fit into your architectural design.

Would the "some conditions" vary based on the logged-in user or other data in the HTTP request?

There are three usecases we are working on right now:

• Vary data and presentation based on User type after authentication.
  While in offline mode we want cached pages and blocks to behave correctly, so we can't rely on BE to return different content based on user state for cms content. This brings inconsistency between online and offline.
• Vary data and presentation based on store config.
  Similar to above, behaviour of our application is based on current store and at this point it's easier to manage it on application side.
• API Endpoint switch.
  Headless approach allows us to switch between backends without rebuild. This also makes dynamic configuration a simpler approach.

And one major complication, that makes it much harder for us to use BuildBus:

• We have our own build mechanism based in react-scripts, that differs from pwa-studio.
  which still allows us to compile page builder, but would require changes to integrate with BuildBus concept.

Possible alternatives we considered, that include build time approach:

• Vary element type on BE based on user type.
  As said above - creates inconsistency for different app modes.
• An aggregation of two different components that will resolve based on User data.
  This increases development complexity, where we have to introduce multiple components for one PageBuilder type on BE. Also I really like the data flow, where we have one configuration on admin side, one config aggregator that is responsible for mapping and one presentation component for this config.

In the end we decided, that dynamic modification of default config is the easiest and the most transparent way of dealing with our requirements.
Perhaps you have better ideas on how we can effectively implement our part, so it better works with your architectural design?

[vchirkov]

@zetlen, @davemacaulay
Hello, gents, it's been a while, I've updated MR to latest version, do you have any concerns or we can proceed with merging in? :)

[zetlen]

@vchirkov Thanks for your patience. You've been very responsive on this ticket. On the other hand, we have let it lie fallow here on the core team, partly because we've been busy defining the parameters of our extension framework in the first place, and deciding what runtime operations need to be necessary.

I think we should pass this code through another round of review (@davemacaulay and @sirugh and @jimbo perhaps) and then strongly consider merging it.

Why to do it

I'm very receptive to concerns about changing app configuration and content without doing another rebuild and push. Eventually, our ideal architecture will allow incremental rebuilds of the app, and once those rebuilds are available and cheap, we'd want to do most of our configuration changes that way.

But until then, it's reasonable to want to change app behavior based on environmental settings. This use case clinches it for me:

• Vary data and presentation based on store config.
  Behaviour of our application is based on current store and at this point it's easier to manage it on application side.

Your goal is to use the same bundled application on different stores. Different stores may have different PageBuilder default configurations. Not all of the PageBuilder configuration is embedded into actual PageBuilder content, so there is a clear place where you may need to mutate PageBuilder configuration at load time.

This use case is less persuasive to me:

• API Endpoint switch.
  Headless approach allows us to switch between backends without rebuild. This also makes dynamic configuration a simpler approach.

Right now, we do hardcode the MAGENTO_BACKEND_URL into the app under some circumstances. That can change, though; and if we make it more dynamic, we can then say that the backend configuration is driven entirely by environment variables, and you could in fact deploy the same app on different websites today simply by changing MAGENTO_BACKEND_URL.

And finally, this use case brings up a question for me:

• Vary data and presentation based on User type after authentication.
  While in offline mode we want cached pages and blocks to behave correctly, so we can't rely on BE to return different content based on user state for cms content. This brings inconsistency between online and offline.

When offline, the Apollo client uses its local cache to simulate request responses. If the user was logged in before, they should see the same content and data that was already cached. If you're designing an offline mode that doesn't use Apollo cache, or tries to simulate responses by other means, then I suppose you'd have to reimplement user state. You shouldn't have to do that, though; please let us know if Apollo local cache is not suiting your purposes.

[vchirkov]

Thanks James, sorry for the long answer, missed the notification.
@davemacaulay, @sirugh, @jimbo may I ask you to have one more look at it, please? At this point lack this functionality turns into blocker for us.

@zetlen, regarding you last question, you are absolutely right and I tried to say the same, just didn't express myself well. What I tried to say is that one of possible alternatives potentially could be to return different pagebuilder config from server, but as you said that won't work because of apollo cache.

[vchirkov]

Hello, gents, As I see according to ci bot all checks have passed.
Is there anything else required from me?
And is there publish date already defined, so we can plan our work accordingly?

[zetlen]

I'm going to move this into "Ready for QA" status so that @dpatil-magento can run our full regression suite on it! Thank you @vchirkov

[vchirkov]

@zetlen, thanks for your help!