[SPIKE] Extension via Portals #1894

sirugh · 2019-10-15T16:09:31Z

Extensions live in client code (ie. venia-concept)
Extensions should be added within providers (how do we handle form context providers?)
Extension base component could be HOC but implemented as wrapper here in venia-ui.

Still much to do, but this is a starting point.

PWAStudioBot · 2019-10-15T16:13:44Z

	Fails
🚫	Missing "Description" section. Please add it back, with detail.
🚫	Missing "Verification Steps" section. Please add it back, with detail.
🚫	A version label is required. A maintainer must add one.

	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).

If your PR is missing information, check against the original template here. At a minimum you must have the section headers from the template and provide some information in each section.

Generated by 🚫 dangerJS against 70fe646

sirugh · 2019-10-15T19:22:31Z

packages/venia-concept/src/extensions/WelcomeToast/WelcomeToast.js

+    }, [addToast, signedIn, firstname, lastname]);
+
+    // Gotta return something or React complains.
+    return <></>;


Can we get around having to return something? This has to be either a fragment or null.

If there's no element to return, return null.

sirugh · 2019-10-15T19:49:19Z

packages/venia-ui/lib/components/Extension/extension.js

+    return isMounted && !!extensionPoint
+        ? createPortal(<Fragment>{children}</Fragment>, extensionPoint)
+        : null;
+};


Reading up on Portals it seems there is an alternative method to mounting which doesn't require the target to have been mounted. The following does still wait to append/remove the extension DOM until mount but it immediately creates the DOM node on which to stick the children. I think we should stick with what we have as it's probably a better experience for extension developers who may want to access state/size/etc but I just wanted to point out an alternative render approach.

export const Extension = props => { const { children, targetId = 'root' } = props; const extensionRoot = useMemo(() => document.createElement('div'), []); useEffect(() => { const extensionPoint = document.getElementById(targetId); extensionPoint.appendChild(extensionRoot); return () => { extensionPoint.removeChild(extensionRoot); } }, [extensionRoot, targetId]); return createPortal(children, extensionRoot); };

Someone discussed the pros/cons. It really comes down to the expectation of how extensions should behave/interact with each other and venia.

revanth0212 · 2019-11-01T16:30:56Z

packages/venia-concept/src/index.js

@@ -46,6 +50,25 @@ ReactDOM.render(
    <Adapter apiBase={apiBase} apollo={{ link: apolloLink }} store={store}>
        <AppContextProvider>
            <App />
+            {/* Some extensions require a target */}
+            <Extension targetId="main-ep-before-children">


What is your opinion about moving this extension creation logic to a file of its own, maybe SampleExtension.js and just use <SampleExtension /> here?

Obviously this is just an example but I am curious to know what you think of it?

My point is, there will be less change in the venia-concept related files making things simpler to maintain custom code.

revanth0212

I like the API for creating extensions. This is a nice way to hook into the DOM without touching a lot of venia-concept code.

This definitely works and scales for DOM dependent/manipulating extensions.

supernova-at · 2019-11-05T14:27:06Z

packages/venia-ui/lib/components/Main/main.js

@@ -19,7 +19,11 @@ const Main = props => {
    return (
        <main className={rootClass}>
            <Header />
-            <div className={pageClass}>{children}</div>
+            <div className={pageClass}>
+                <div id="main-ep-before-children" />


I'm fearful these extension points will get out of control.

We'll have to add them all over the place

That still won't satisfy every use case our clients want

Seemingly innocuous changes (I'm thinking pageClass turns from flex to grid or something) could result in completely broken extensions that our versioning strategy is never going to be able to account for.

I'm fearful these extension points will get out of control.

Me too man, me too.

We'll have to add them all over the place

I don't think so. There's whitespace but there's not that much whitespace. If you break the app down into higher level components you only have maybe 5-10 components/pages we'd have to add these to. These would all be documented clearly anyways.

That still won't satisfy every use case our clients want

Yea -- but I don't think that's really something we should solve for. We are in the business of providing a pretty flexible template/theme for use out of the box with minimal necessity for customization and I think this would be a good starting point.

Seemingly innocuous changes (I'm thinking pageClass turns from flex to grid or something) could result in completely broken extensions that our versioning strategy is never going to be able to account for.

We might need to do something like make extension nodes position: relative. I wish I had a better grasp on how we could protect these nodes from our own styling.

We'll have to add them all over the place [and] that still won't satisfy every use case our clients want

Like @sirugh said, we're just exploring happy paths for adding content to a storefront. We would only put extension points in the safest places to add content.

Seemingly innocuous changes (I'm thinking pageClass turns from flex to grid or something) could result in completely broken extensions that our versioning strategy is never going to be able to account for.

This is true. In a normal app where users write global CSS targeting any classname, we'd assume that after adding content to an extension point, they'd write a CSS rule targeting its parent to ensure it was laid out correctly. This use case gets tougher with scoped CSS, though, since a user would have to take over the parent (defeating the purpose of this happy path).

I'm fearful these extension points will get out of control.

Slight cringe while reading this and hearing Ron Howard say "They did." Magento always had bottomless XML nodes that render any child, and later added a few explicit "extension placeholder" nodes.

In full honesty, I always hated these because when extensions hook onto them it never seems to be "the right place" for that extension — just an inoffensive place to put anything. Factor in responsive design (they didn't) and you always-always had to manually inject them somewhere else.

I understand it breaks the plug-n-play goal of cloud+extensions but I would've been happier to just see extensions define an explicit hook for itself and tell you (the developer integrating it) to add that one-line-of-code where it makes sense. The "safest places to add content" just weren't useful in the real client work.

I understand it breaks the plug-n-play goal of cloud+extensions but I would've been happier to just see extensions define an explicit hook for itself and tell you (the developer integrating it) to add that one-line-of-code where it makes sense. The "safest places to add content" just weren't useful in the real client work.

I agree, and my personal experience as an agency developer is in line with yours: for the best results, "real client work" should consist of developers manually integrating changes, regardless of whether those changes incorporate third-party code. Extensions that effect a seamless change are pretty rare in reality, even if they're targeting hard extension points.

That said, the plug & play extension story happens to be important to a lot of potential PWA Studio users. The lack of automatic extension integration seems to be holding some people back from trying out PWA Studio, even though they would likely find that manual integration leads to better experiences for shoppers and developers alike. A basic extension system would probably help us ease more people into this new ecosystem.

Zooming out, though, any approach to extensions would result in one of two outcomes:

If extensions are limited to only a few explicit placeholders, they'll be too limited for most scenarios, resulting in developers having to manually integrate their customizations anyway.

If extensions are not limited to a few explicit places, extensions will be able to undermine most assumptions about the page structure, resulting in defects and a general lack of predictability.

The latter is a net negative that we want to avoid. But the former should be able to provide risk-free value, even if it's only a small amount. If we can find a way to do this risk-free, we'll do it; if we can't, we won't do it.

supernova-at · 2019-11-05T14:28:49Z

packages/venia-concept/src/index.js

+            {/* Other extensions don't need target (assumes #root at least) */}
+            <Extension>
+                <WelcomeToast />
+            </Extension>


This is super cool

Nice work covering the different use cases. 💯

supernova-at · 2019-11-05T14:30:21Z

packages/venia-ui/lib/components/Extension/extension.js

+    // Only render after the initial mount, and only if the extension point has
+    // been found.
+    return isMounted && !!extensionPoint
+        ? createPortal(<Fragment>{children}</Fragment>, extensionPoint)


I didn't realize createPortal was so easy - nice 💪

The cool part is that the portal should render as soon as extensionPoint exists—without us having to find some lifecycle event or a callback that fires right when extensionPoint is created.

We might have to think about how often it's calling document.getElementById() though.

jimbo

This is a good exploration of this concept. I like how much value we get for so little effort and disruption.

I'm ready to archive this. We should keep an index of these spikes in an issue.

jimbo · 2019-11-05T17:18:39Z

packages/venia-concept/src/extensions/WelcomeToast/WelcomeToast.js

+    }, [addToast, signedIn, firstname, lastname]);
+
+    // Gotta return something or React complains.
+    return <></>;


If there's no element to return, return null.

jimbo · 2019-11-05T17:22:05Z

packages/venia-concept/src/index.js

+            {/* Other extensions don't need target (assumes #root at least) */}
+            <Extension>
+                <WelcomeToast />
+            </Extension>


Nice work covering the different use cases. 💯

jimbo · 2019-11-05T17:25:44Z

packages/venia-ui/lib/components/Extension/extension.js

+    // Only render after the initial mount, and only if the extension point has
+    // been found.
+    return isMounted && !!extensionPoint
+        ? createPortal(<Fragment>{children}</Fragment>, extensionPoint)


The cool part is that the portal should render as soon as extensionPoint exists—without us having to find some lifecycle event or a callback that fires right when extensionPoint is created.

We might have to think about how often it's calling document.getElementById() though.

jimbo · 2019-11-05T17:32:14Z

packages/venia-ui/lib/components/Main/main.js

@@ -19,7 +19,11 @@ const Main = props => {
    return (
        <main className={rootClass}>
            <Header />
-            <div className={pageClass}>{children}</div>
+            <div className={pageClass}>
+                <div id="main-ep-before-children" />


We'll have to add them all over the place [and] that still won't satisfy every use case our clients want

Like @sirugh said, we're just exploring happy paths for adding content to a storefront. We would only put extension points in the safest places to add content.

Seemingly innocuous changes (I'm thinking pageClass turns from flex to grid or something) could result in completely broken extensions that our versioning strategy is never going to be able to account for.

This is true. In a normal app where users write global CSS targeting any classname, we'd assume that after adding content to an extension point, they'd write a CSS rule targeting its parent to ensure it was laid out correctly. This use case gets tougher with scoped CSS, though, since a user would have to take over the parent (defeating the purpose of this happy path).

cherdman · 2019-12-11T17:08:53Z

@revanth0212 and @zetlen , I am closing out the stale spike.

PWAStudioBot added pkg:venia-concept pkg:venia-ui labels Oct 15, 2019

sirugh changed the title ~~Initial commit with some examples of extensions~~ [SPIKE] Extension via Portals Oct 15, 2019

sirugh added 2 commits October 15, 2019 13:49

Initial commit with some examples of extensions

ab82534

Default root extension point

67e0876

sirugh force-pushed the rugh/extension-portal-spike branch from 0f8f4f8 to 67e0876 Compare October 15, 2019 18:49

Update examples with before or after syntax

70fe646

sirugh commented Oct 15, 2019

View reviewed changes

m2-community-project bot assigned sirugh Oct 15, 2019

m2-community-project bot added the Progress: review label Oct 15, 2019

sirugh commented Oct 15, 2019

View reviewed changes

m2-community-project bot removed the Progress: review label Oct 16, 2019

sirugh mentioned this pull request Oct 16, 2019

[SPIKE]: Extensibility via React Portals #1892

Closed

sirugh added this to the Extensibility Framework milestone Oct 22, 2019

revanth0212 reviewed Nov 1, 2019

View reviewed changes

m2-community-project bot assigned revanth0212 Nov 1, 2019

m2-community-project bot added the Progress: review label Nov 1, 2019

revanth0212 approved these changes Nov 1, 2019

View reviewed changes

revanth0212 added the Don't Merge label Nov 1, 2019

sirugh marked this pull request as ready for review November 1, 2019 17:05

supernova-at reviewed Nov 5, 2019

View reviewed changes

m2-community-project bot assigned supernova-at Nov 5, 2019

jimbo reviewed Nov 5, 2019

View reviewed changes

m2-community-project bot assigned jimbo Nov 5, 2019

jimbo added the research Research spike or experimental work. label Nov 7, 2019

cherdman closed this Dec 11, 2019

sirugh deleted the rugh/extension-portal-spike branch March 4, 2020 18:43

[SPIKE] Extension via Portals #1894

[SPIKE] Extension via Portals #1894

Uh oh!

Conversation

sirugh commented Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PWAStudioBot commented Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sirugh Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

revanth0212 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimbo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cherdman commented Dec 11, 2019

Uh oh!

Uh oh!

sirugh commented Oct 15, 2019 •

edited

Loading

PWAStudioBot commented Oct 15, 2019 •

edited

Loading

sirugh Oct 15, 2019 •

edited

Loading