[PWA-334] Implement PageBuilder as an extension #2137

zetlen · 2020-02-03T17:36:42Z

Description

PageBuilder has moved into its own package @magento/pagebuilder. It now integrates into Venia using a brand new plugin pipeline!

Dependencies

The @magento/venia-ui package does not depend on @magento/pagebuilder. All direct references to @magento/pagebuilder in that package have been removed. Instead, @magento/venia-ui exposes a new extension point and PageBuilder integrates into it.

The new @magento/pagebuilder package depends on @magento/venia-ui as a peerDependency. It contains a few direct imports of @magento/venia-ui components in its code. We want PageBuilder to be independent of Venia in the future, but we need to refactor its code to be more independent first.

The @magento/venia-concept package depends on @magento/pagebuilder and @magento/venia-ui. Venia "installs" @magento/pagebuilder just by putting it in its dependencies list!

This satisfies the use case where a developer should be able to say npm install @magento/pagebuilder and it will automatically integrate into their UI on the next build. Magento!

Extension Model

The extension model works entirely at build time, making it superior to other extension systems in performance and bundle size. It's powered by a new concept called the BuildBus. The BuildBus is an object much like an event bus, but it uses the interceptor patterns from Tapable to enable modules to declare, and share, extension points. This is good because it's the same as the popular, successful system in the build tool we already use: Webpack compiler and compilation hooks. (In our project we will call these objects "targets", to avoid confusion with the substantially different concept of React hooks.)

Modules which are extensions can integrate with BuildBus by putting a custom property pwa-studio in their package.json files:

{
  "name": "@magento/venia-ui",
  [...other properties]
  "pwa-studio": {
    "targets": {
      "declare": "./targets/declare",
      "intercept": "./targets/intercept"
    }
  }

The above configuration tells BuildBus that this package both declares its own targets and intercepts other targets. Specifically:

BuildBus will first run the file at @magento/venia-ui/targets/declare. There, Venia UI exposes its own targets for other extensions to modify it.
BuildBus will then run the file at @magento/venia-ui/targets/intercept. There, Venia UI injects itself into other modules' targets, and invokes its own targets to enable other modules to interact with its build.

BuildBus does the following:

runs all modules in dependency order (e.g. @magento/pagebuilder runs after @magento/venia-ui
runs all "declare" files in order, then all "intercept" files in order, so that extensions are guaranteed access to all other targets, even dependents' targets
expects each "declare" and "intercept" file to export a default function, which it will call with a special API.

BuildBusStop API

Declare files and intercept files have access to a "BuildBusStop" facade for the BuildBus, so that BuildBus can track their requests and declarations. The facade has two methods:
api.declareTarget(targetName, target): Declare a named target that other modules can use. targetName must be a string, which will be the public name of this extension point. target must be an instance of a Tapable class.
api.getTarget(moduleName, targetName): Retrieve a target from your own declared targets or another module. Calling api.getTarget() with one argument will try to retrieve the current module's named target. Calling api.getTarget() with two arguments will try to retrieve the targets from a different module.

Once the targets are retrieved, they can be tapped in many ways; see the Tapable docs for the complete API.

Declaring and Intercepting Targets

Modules which declare targets can choose how to invoke those targets. This should usually happen in the intercept phase: for a module's target to do anything, it must in turn intercept an upstream target in one of its dependencies, and in its own handler it can invoke its named target with any arguments it chooses.

Current Extension Points

(The list will get a lot longer; this PR only implements enough extension points to do a PageBuilder integration.)

BuildBus

BuildBus has some builtin targets; these targets hook directly in to the build process, and all extension targets are ultimately powered by interceptors on these base targets.

specialFeatures: Runs before the compilation begins. Interceptors will receive the special configuration object that the root project argued to configureWebpack(). They can add to that object, indicating that the current package contains esModules, cssModules, rootComponents, et cetera.
webpackCompiler: Runs when a Webpack compiler is created. Interceptors will receive the compiler object, which has a hook property containing Tapable objects with the exact same API as BuildBus targets. Interceptors of webpackCompiler will usually tap into the compiler's hooks, which allows them to interact with the build.

Venia UI

Venia UI declares a richContentRenderers target. Interceptors to this target receive an API object which allows them to add additional content renderers to the RichContent component. Each content renderer must be a module that exposes a canRender function, which receives CMS content data and should return true if the renderer can display it, and a Component React component, which will be used to render. These renderers are added to a list in dependency order, where the first one which can render will be used. It builds that list manually by editing source code at build time, injecting direct references to all rich content renderers.

Related Issue

Closes [PWA-334]..

Acceptance

Verification Stakeholders

@davemacaulay
@jimbo
@sirugh
@supernova-at
@revanth0212
@jcalcaben
@tjwiebell
@awilcoxa

Specification

PageBuilder functionality must be reviewed.
Fallback HTML render must be reviewed.
Build times and bundle sizes must be analyzed and compared.

Verification Steps

Set your Venia instance MAGENTO_BACKEND_URL to https://dave-mhvaqay-vzsrtettsztvg.us-4.magentosite.cloud/ to access an instance with PageBuilder data on the home page.
Manually remove @magento/pagebuilder from packages/venia-concept/package.json.
Run yarn watch:venia.
Visit the homepage.
Verify that content exists, but it's not attractively styled as hero images by PageBuilder. Scroll around and look for a parallax effect; it should not be there.
Add @magento/pagebuilder back to the dependencies list in packages/venia-concept/package.json.
Run yarn watch:venia.
Visit the homepage.
Verify PageBuilder content renders beautifully..
Run a full production build and verify staging as well.

Checklist

I have updated the documentation accordingly, if necessary.
I have added tests to cover my changes, if necessary.

PWAStudioBot · 2020-02-03T17:43:14Z

	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).
📖	Associated JIRA tickets: PWA-334.

Generated by 🚫 dangerJS against ea6d6e0

feat(extensibility): pagebuilder is a separate module test: coverage for new pagebuilder test: BuildBus coverage

brendanfalkowski · 2020-02-03T21:14:38Z

Our team started testing PageBuilder in PWA last week, and I'm collating some questions we had after seeing these changes.

Question One

PageBuilder components seemed to be re-redefined in three places, and now four potentially.

code for the Magento admin
code for the PHTML frontend (not a concern for us)
code for the PWA frontend (in /components/RichContent)

Now (3) is split into:

code for the PWA frontend (from @magento/pagebuilder)
code for the PWA frontend (that customizes @magento/pagebuilder or is new)

We DO NOT have a significant amount of content to migrate, and could ignore PageBuilder (or CMS entirely by deploying content as code) as a shortcut. What we don't want to do is sink time into modifying PageBuilder components in PWA or PHP then see a consolidation of these redefinitions.

As of this PR, we're not sure if we should freeze our work customizing in the RichContent component because it's not clear how we'll extend this once it's not our source, but an NPM dependency. Any advice?

Question Two

In the PageBuilder RichContent component, we see generic UI (e.g. button, heading, tabs, etc) that already exists in components but instead borrows the PHP version's makeup. It makes sense that PageBuilder components have a prop API matching the PageBuilder admin, but we prefer they share the design system of our tailored application components.

So it looks like two paradigms are implied "application components" + "CMS components" and they don't share. Is this an intentional split for separate concerns? Again we don't really want to customize heavily if there's a long-term consolidation planned, but we don't expect to be able to use @magento/pagebuilder imports without a mechanism to apply our stylesheet whether we maintain it in one place or two.

fooman · 2020-02-03T22:42:59Z

packages/venia-ui/targets/intercept.js

+
+module.exports = api => {
+    const richContentRenderers = api.getTarget('richContentRenderers');
+    api.getTarget('@magento/pwa-buildpack', 'webpackCompiler').tap(


I imagine for other parts of the code that declare their own targets in the future this implementation of handling the target will look very similar and won't change that much. Any chance this can be generalised so it can be more easily reused?

There's more than a chance; there's a plan! We plan to leave these code snippets in place for a while, as we implement more and more functionality as extensions. As patterns appear, we will abstract them into a library of extension utility functions. This way, we can grow towards a more terse, declarative model, while still supporting lower-level customizations.

jimbo · 2020-02-03T23:18:30Z

So it looks like two paradigms are implied "application components" + "CMS components" and they don't share. Is this an intentional split for separate concerns? Again we don't really want to customize heavily if there's a long-term consolidation planned

Astute observation.

Venia and PageBuilder components are separate because they're two existing, independent systems that work differently. They're not separate concerns—both are responsible for rendering interactive, responsive UI—but they differ on a few core implementation principles.

PageBuilder uses a blend of global styles and inline styles to achieve the final presentation, and many of those inline styles are stored as data and passed down the tree as component props.
Venia uses scoped styles and custom properties to achieve the final presentation, so the component code often knows little about the appearance of the end result.

It would be hard to consolidate these components without reconciling these differences and others. For example, PageBuilder's browser support matrix includes Internet Explorer versions that Venia's does not. This doesn't mean consolidating our components is impossible, just that it would amount to a refactor of PageBuilder's rendering code (major version, regression, etc.), which is not something we're undertaking at the moment.

That said, there's opportunity for it to happen. We'll be expanding Venia's usage of custom properties soon, so that style values (e.g., colors) will properly derive from design tokens. We're doing that to offer an easier way to "theme" Venia, but it'll also do a better job of exposing our design architecture to PageBuilder and other extensions—and they may opt to make use of those properties.

jimbo

Awesome. 166 files touched would seem like a lot, but it's almost entirely renames and fixing imports. I've left a few small questions in places, but this is very cool.

jimbo · 2020-02-04T22:34:48Z

packages/venia-ui/lib/components/RichContent/__tests__/richContent.spec.js

 test('renders a RichContent component', () => {
    const component = createTestInstance(<RichContent />);

    expect(component.toJSON()).toMatchSnapshot();
 });

-test('renders a RichContent component with Page Builder content', () => {


Are these tests obsolete?

jimbo · 2020-02-04T22:42:45Z

packages/venia-concept/package.json

@@ -12,7 +12,7 @@
  "scripts": {
    "build": "yarn run build:prod",
    "build:analyze": "yarn run clean && mkdir dist && webpack -p --profile --no-progress --env.mode production --json > dist/build-stats.json && webpack-bundle-analyzer dist/build-stats.json",
-    "build:dev": "yarn run clean && yarn run validate-queries && webpack --progress --env.mode development",
+    "build:dev": "yarn run clean && webpack --no-progress --env.mode development",


Reasoning behind this change?

I have a reason, but it's not relevant to this PR, so I'm reverting.

jimbo · 2020-02-04T22:42:56Z

packages/venia-concept/package.json

@@ -49,6 +49,7 @@
    "@babel/runtime": "~7.4.2",
    "@magento/babel-preset-peregrine": "~1.0.1",
    "@magento/eslint-config": "~1.5.0",
+    "@magento/pagebuilder": "~1.0.0",


jimbo · 2020-02-04T22:43:20Z

packages/venia-concept/package.json

@@ -88,6 +89,9 @@
    "graphql-tag": "~2.10.1",
    "html-webpack-plugin": "~3.2.0",
    "informed": "~2.1.13",
+    "jarallax": "~1.11.1",
+    "load-google-maps-api": "~2.0.1",
+    "lodash.escape": "~4.0.1",


Why do we have to add PB's dependencies here?

I'm inclined to declare as many dependencies as possible as peerDependencies, which is what they are in @magento/pagebuilder. That's a safer model, to me, for building an efficient app. It enforces that only one copy of jarallax and other such libraries can be in the same bundle at one time.

When a dev installs @magento/pagebuilder, they'll see warnings that they also need to add these packages as dependencies. So then they can, and they'll have control over those package versions. If another package wants an incompatible version of jarallax, it'll fail the build, rather than silently putting two copies of the same library into the bundle.

Ah, I see why. If these are peer dependenies of @magento/pagebuilder, that does explain why they would be listed as dependencies of @magento/venia-concept. Wouldn't they be real dependencies here, though, rather than dev dependencies?

@zetlen if the application did not have @magento/pagebuilder installed, would the bundle still include Jarallax etc? Or would these only be included if the bundler discovered their usage in the application?

I'm inclined to say, any package that is optional / installable should own it's dependencies and they should be automatically installed alongside the requirement of the package. If you didn't have @magento/pagebuilder installed seeing these in the @magento/venia-concept package is a little strange.

@davemacaulay That is an interesting question. In this peerDeps driven workflow, here's what the workflow would be:

Without PageBuilder installed, none of those other deps are installed either

yarn add @magento/pagebuilder

Install process shows warnings about installing jarallax and other dependencies

Build fails if those dependencies are not installed

Developer must install those deps as well

This happens with Babel and other common tools, but I would understand if it feels too complex for a new NPM/yarn user. There may be other good ways to prevent multiple copies of a library in the same bundle; do you have an alternative suggestion?

@zetlen I guess I'm used to the world of composer where it'll automatically resolve and install all dependencies on install, but you're right in saying if they required different versions things would get a little messy.

I don't have an alternative and if this is the way to go it makes enough sense to me. I'd be inclined to say we shouldn't include the deps for Page Builder in venia-concept, as any other package not authored by us won't have this luxury, but as we're going to include @magento/pagebuilder by default for now we probably have too.

If you have any suggestions of how we can avoid this that'd be great, but not necessary for this iteration.

jimbo · 2020-02-04T23:01:27Z

packages/pwa-buildpack/lib/WebpackTools/plugins/BuildBusPlugin.js

@@ -0,0 +1,13 @@
+class BuildBusPlugin {
+    constructor(bus) {
+        this.bus = bus;


No default value necessary?

Nah, nobody's creating these but us.

jimbo · 2020-02-04T23:12:20Z

packages/venia-ui/lib/components/RichContent/richContent.js

-            <div className={classes.root}>
-                <PageBuilder masterFormat={html} />
-            </div>
+    const newProps = {


Consider avoiding new in vars. 👍

Suggested change

const newProps = {

const nextProps = {

Thanks. Renamed to be more specific.

jimbo · 2020-02-04T23:20:53Z

packages/pagebuilder/package.json

+  },
+  "homepage": "https://github.com/magento-research/pwa-studio/tree/master/packages/pagebuilder#readme",
+  "dependencies": {},
+  "devDependencies": {


This seems inaccurate to me. I would expect most of these to be actual dependencies, not dev dependencies. No?

Related, are you using NPM instead of Yarn?

In the core repo I'm using Yarn, but this system works with either.

Also, all of the dependencies that PageBuilder needs at runtime are listed as peerDependencies, not literal dependencies. See my other comment for an explanation of that.

It enforces that only one copy of jarallax and other such libraries can be in the same bundle at one time.

The NPM/Yarn model is built totally around the idea of tolerating mutually incompatible versions of a library. That's great for Node, and browser apps without strict performance budgets, but we need to be more careful--and peer deps are the NPM-supported way of doing that.

jimbo · 2020-02-04T23:44:33Z

packages/pagebuilder/intercept.js

+            richContentRenderers.add('PageBuilder', '@magento/pagebuilder');
+        }
+    );
+};


This is really concise.

tjwiebell · 2020-02-05T19:38:27Z

packages/pagebuilder/lib/ContentTypes/Products/products.css

@@ -26,7 +26,7 @@
 }

 .galleryItems {
-    composes: items from '../../../../Gallery/gallery.css';
+    composes: items from '~@magento/venia-ui/lib/components/Gallery/gallery.css';


Accidental tilde?

Accidental tilde?

Not accidental. Required for absolute node_modules refs since [email protected].

davemacaulay · 2020-02-11T20:23:45Z

@zetlen I love the new API, the method calls and reasoning behind the changes make a lot of sense, I'm happy to move ahead with making those alterations if everyone else is.

fooman · 2020-02-12T04:13:47Z

targets.of('@magento/venia-ui').richContentRenderers does look a lot easier to grasp. If I am not mistaken the use of targets.own will ensure that different packages can't interfere with each others extensions points as easily. This would be a win as well.

zetlen · 2020-02-13T15:41:37Z

@fooman Yes, that's the idea. There exist "backdoors" for packages to inspect each others' extension points, but this API has a few safeguards for that:

The declare and intercept phases run sequentially, and the packages run in dependency order
The targets API passed to each package's declare and intercept files is actually a facade object. It's bound to that particular package and it tracks any interactions between that package and others.

With this architecture we can add rules in the future pretty easily, such as preventing any package from running call on another package's interceptor.

dpatil-magento · 2020-02-14T20:19:11Z

@zetlen Ran through verification steps looks good. Stage server looking good.
PWA - https://pr-2137.pwa-venia.com/
PB-PWA - https://pr-2168.pwa-venia.com/. (Built on your branch)

So pending issue is about env variables, ex - GOOGLE_MAPS_API_KEY. Also one more minor thing package.json has reference to magento-research/pwa-studio instead of magento/pwa-studio.

@dhaecker Would you like to take a quick look?

zetlen · 2020-02-18T15:35:54Z

To support extensible environment variables, I need to add overloads to some public methods. My changeset has gotten large; we can either merge a lot of complex code or make this another major release, so I can introduce breaking changes to Buildpack APIs. @dpatil-magento we can talk about this internally.

dpatil-magento · 2020-02-18T17:07:03Z

Created bug https://jira.corp.magento.com/browse/PWA-379 for env variable issue.

Its good to merge once @dhaecker approves.

dhaecker · 2020-02-18T17:18:46Z

@dpatil-magento have you tested anything page builder related in this PR (what have you tested)? Thank you

dpatil-magento · 2020-02-18T17:24:53Z

@dhaecker I had deployed these changes to https://pr-2168.pwa-venia.com/ and compared home page with https://pb.pwa-venia.com/ on Chrome.

dhaecker · 2020-02-18T17:30:41Z

@dpatil-magento After speaking with Dave M. about the changes, no functionality is expected to change in Page Builder content so your testing is good enough. I approve ✅

zetlen requested a review from davemacaulay as a code owner February 3, 2020 17:36

PWAStudioBot added pkg:pwa-buildpack pkg:venia-concept pkg:venia-ui pkg:pagebuilder labels Feb 3, 2020

feat(buildpack): Implement PageBuilder as extension. Closes [PWA-334]

636d947

feat(extensibility): pagebuilder is a separate module test: coverage for new pagebuilder test: BuildBus coverage

zetlen force-pushed the zetlen/pagebuilder-as-plugin branch from e21a4b8 to 636d947 Compare February 3, 2020 19:16

fixup path bug

2e8cd2d

fooman reviewed Feb 3, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

zetlen mentioned this pull request Feb 4, 2020

DO NOT MERGE: feat(buildpack): Scaffolding tools for composing templates #1888

Closed

jimbo suggested changes Feb 4, 2020

View reviewed changes

m2-community-project bot assigned jimbo Feb 4, 2020

m2-community-project bot added the Progress: changes requested label Feb 4, 2020

zetlen added the version: Minor This changeset includes functionality added in a backwards compatible manner. label Feb 5, 2020

Merge branch 'develop' into zetlen/pagebuilder-as-plugin

060ccf1

tjwiebell reviewed Feb 5, 2020

View reviewed changes

m2-community-project bot assigned tjwiebell Feb 5, 2020

m2-community-project bot added Progress: review and removed Progress: changes requested labels Feb 5, 2020

chore: update dockerfiles with new package

ca78d3f

m2-community-project bot assigned zetlen Feb 5, 2020

style: avoid new keyword in var name

8c58208

sirugh self-requested a review February 5, 2020 23:03

fixup tests

b5ce559

James Zetlen added 4 commits February 12, 2020 13:46

test: coverage for buildbus refactor

7cff0f5

Merge branch 'develop' into zetlen/buildbus-redux

7dc3c94

Merge branch 'develop' into zetlen/pagebuilder-as-plugin

1202a61

Merge branch 'zetlen/buildbus-redux' into zetlen/pagebuilder-as-plugin

6a628f5

zetlen dismissed jimbo’s stale review via 6a628f5 February 12, 2020 19:47

James Zetlen added 3 commits February 13, 2020 10:11

fixup requestTargets mistake

0597245

fixup bugs in Target facade

cedf27a

fixup buildbusplugin coverage

92146c0

zetlen mentioned this pull request Feb 13, 2020

feat 2130 - ability to add PageBuilder Content Type Configs dynamically #2131

Merged

feat: prevent external targets from running call methods

53495ec

James Zetlen added 2 commits February 16, 2020 10:43

Merge branch 'develop' into zetlen/pagebuilder-as-plugin

d97e57d

fixup update magento-research org

d20a737

PWAStudioBot added the pkg:create-pwa label Feb 17, 2020

Merge branch 'develop' into zetlen/pagebuilder-as-plugin

ea6d6e0

zetlen mentioned this pull request Feb 18, 2020

feat: interception targets for env vars #2174

Merged

davemacaulay approved these changes Feb 18, 2020

View reviewed changes

davemacaulay merged commit f6988f9 into develop Feb 18, 2020

zetlen mentioned this pull request Apr 28, 2020

fix(scaffolding): get base envVarDefinitions if targets don't exist yet #2363

Merged

dpatil-magento deleted the zetlen/pagebuilder-as-plugin branch September 14, 2020 22:05

[PWA-334] Implement PageBuilder as an extension #2137

[PWA-334] Implement PageBuilder as an extension #2137

Conversation

zetlen commented Feb 3, 2020

Description

Dependencies

Extension Model

BuildBusStop API

Declaring and Intercepting Targets

Current Extension Points

BuildBus

Venia UI

Related Issue

Acceptance

Verification Stakeholders

Specification

Verification Steps

Checklist

PWAStudioBot commented Feb 3, 2020 • edited Loading

brendanfalkowski commented Feb 3, 2020 • edited Loading

Question One

Question Two

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimbo commented Feb 3, 2020

This comment has been minimized.

jimbo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimbo Feb 5, 2020 • edited Loading

Choose a reason for hiding this comment

davemacaulay Feb 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davemacaulay commented Feb 11, 2020

fooman commented Feb 12, 2020

zetlen commented Feb 13, 2020

dpatil-magento commented Feb 14, 2020 • edited Loading

zetlen commented Feb 18, 2020

dpatil-magento commented Feb 18, 2020

dhaecker commented Feb 18, 2020

dpatil-magento commented Feb 18, 2020

dhaecker commented Feb 18, 2020

PWAStudioBot commented Feb 3, 2020 •

edited

Loading

brendanfalkowski commented Feb 3, 2020 •

edited

Loading

jimbo Feb 5, 2020 •

edited

Loading

davemacaulay Feb 10, 2020 •

edited

Loading

dpatil-magento commented Feb 14, 2020 •

edited

Loading