#7670: Simplify creation of isolated widgets (`IsolatedComponent`) #8151

fregante · 2024-04-04T11:43:35Z

What does this PR do?

Uses POC: Isolated components #8139
Complements #7670: Simplify injection of widgets onto the page (renderWidget) #8150

You can find the implementation details in the POC PR above.

Future work

Migrate the rest of the EmotionShadowRoot components, some of which might are the reason why this research was necessary (e.g. DocumentView)
- #7670: Use <IsolatedComponent> for DocumentView, EphemeralFormContent, CustomFormComponent #8200

Checklist

Add tests and/or storybook stories
Designate a primary reviewer: @grahamlangford

https://togithub.com/sindresorhus/eslint-plugin-unicorn/issues/1514 https://togithub.com/pixiebrix/pixiebrix-extension/pull/4486

fregante · 2024-04-04T11:49:41Z

jest.config.js

+    "\\.ya?ml$": "yaml-jest-transform",
+    "\\.txt$": "<rootDir>/src/testUtils/rawJestTransformer.mjs",
+    // Note: `?param` URLs aren't supported here: https://github.com/jestjs/jest/pull/6282
+    // You can only use a mock via `moduleNameMapper` for these.


I updated these to avoid confusion. See comment/link.

fregante · 2024-04-04T11:50:11Z

src/bricks/renderers/PropertyTree.tsx

@@ -15,25 +15,22 @@
 * along with this program.  If not, see <http://www.gnu.org/licenses/>.
 */

+import "primereact/resources/themes/saga-blue/theme.css";
+import "primereact/resources/primereact.min.css";
+import "primeicons/primeicons.css";


We can just load the stylesheets normally now 🎉

fregante · 2024-04-04T11:52:12Z

src/components/EmotionShadowRoot.ts

 const ShadowRoot = EmotionShadowRoot.div!;
+// TODO: Use EmotionShadowRoot["pixiebrix-widget"] to avoid any CSS conflicts. Requires snapshot/test updates


I tried this in the POC PR, but it affected too many tests. I can try again later.

fregante · 2024-04-04T11:52:54Z

src/components/IsolatedComponent.scss

+// Don't inherit the selection color
+:host::selection {
+  background: initial;
+}


A full default stylesheet in a documented CSS file 🎉

fregante · 2024-04-04T11:53:29Z

src/components/IsolatedComponent.tsx

+
+// This cannot be a CSS module or URL because it must live inside the
+// shadow DOM and synchronously set the :host element style.
+import cssText from "./IsolatedComponent.scss?loadAsText";


This is the reason why I changed the Jest config

fregante · 2024-04-04T11:54:36Z

src/components/IsolatedComponent.tsx

+  }
+
+  return false;
+}


You can find the context in webpack-contrib/mini-css-extract-plugin#1092. In short, webpack injects the stylesheet in the main document, so we need to target and remove it. This is also mentioned in the code below.

fregante · 2024-04-04T12:02:56Z

src/components/quickBar/QuickBarApp.tsx

-              whichever website it's rendering on.
-              To support react-select and any future potential emotion components we used the
-              emotion variant of the react-shadow library.
-            */}


Moved to EmotionShadowRoot

fregante · 2024-04-04T12:03:59Z

src/components/selectionToolPopover/SelectionToolPopover.module.scss

-  font-weight: 400;
-  line-height: 1.5;
-  color: #212529;
-  letter-spacing: normal;


Nothing lasts longer than quick temporary fixes:

#4440: Selection Tool is not isolated from page style #4486 (comment)

This was resolved by:

#7670: Remove rem units from bootstrap to avoid widget conflicts #7940

I dropped it here because the file is now an actual CSS module so the classes had to change.

codecov · 2024-04-06T10:12:26Z

Codecov Report

Attention: Patch coverage is 37.14286% with 44 lines in your changes are missing coverage. Please review.

Project coverage is 73.38%. Comparing base (c295c6c) to head (efb53fe).
Report is 1 commits behind head on main.

❗ Current head efb53fe differs from pull request most recent head 1334a91. Consider uploading reports for the commit 1334a91 to get more accurate results

Files	Patch %	Lines
src/components/IsolatedComponent.tsx	22.22%	21 Missing ⚠️
...ents/selectionToolPopover/SelectionToolPopover.tsx	25.00%	9 Missing ⚠️
src/components/Stylesheets.tsx	52.94%	8 Missing ⚠️
.../selectionToolPopover/showSelectionToolPopover.tsx	50.00%	4 Missing ⚠️
src/bricks/renderers/propertyTable.tsx	50.00%	1 Missing ⚠️
...ript/textSelectionMenu/selectionMenuController.tsx	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8151      +/-   ##
==========================================
- Coverage   73.43%   73.38%   -0.05%     
==========================================
  Files        1317     1315       -2     
  Lines       40780    40762      -18     
  Branches     7570     7579       +9     
==========================================
- Hits        29947    29915      -32     
- Misses      10833    10847      +14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

fregante · 2024-04-06T11:31:30Z

src/__snapshots__/Storyshots.test.js.snap

+    style={
+      {
+        "fontSize": "16px",
+      }


🆕 The storybook now works, however the component includes absolute positioning so it's cut. It can be adjusted separately.

fregante · 2024-04-06T11:56:37Z

src/bricks/renderers/propertyTable.tsx

+      <IsolatedComponent
+        name="PropertyTree"
+        lazy={async () =>
+          import(
+            /* webpackChunkName: "isolated/PropertyTree" */
+            "./PropertyTree"
+          )
+        }
+        factory={(PropertyTree) => <PropertyTree value={value} />}
+      />


This is the format I ended up with, I think it's compact and readable enough, which is important due to all the restrictions:

name, isolated/${name}, "/${name}" must all match. Unfortunately this cannot use a variable since they're used by webpack. I can add a lint rule though.

the imported package must also appear in the isolatedComponentList.mjs file, which is picked up by webpack, by <IsolatedComponent/> itself for verification, and by ESLint eventually

the lazy/factory attribute names are the best I could come up with. suggestions welcome (one is React.lazy(LAZY) and the other one returns the result of COMPONENT in const COMPONENT = React.lazy(LAZY)

I'd like to document some of these requirements all in one place somewhere (e.g. it's not clear just looking at IsolatedComponent.tsx that you'd need to add that component to isolatedComponentList). Either in the docstring directly or in a README that's referenced in the docstring.

If I'm looking to add a new isolated component, what are the steps that need to be taken and out-of-the box behaviors or assumptions that are made?

Yes definitely it needs some documentation and a lint rule, but:

the JSDoc includes a full example that is identical every time, where only the name and import path need to be changed, usually

if it's used incorrectly, either webpack, typescript or the component itself will throw, depending on what part is wrong

I can't think of anything that breaks that isn't already caught but the build.

assumptions that are made

Since "PropertyTree" is now loaded as an isolated component, it needs to import all the stylesheets it needs, including bootstrap if it needs it (it doesn't in this case). No component should need to use <Stylesheets/> directly unless it's loading the user's stylesheets.

I'd like to document some of these requirements all in one place somewhere

I just checked, each prop is already documented and it includes the requirement:

pixiebrix-extension/src/components/IsolatedComponent.tsx

Lines 61 to 70 in 1334a91

/**

* It must match the `import()`ed component's filename

*/

name: string;

/**

* It must follow the format `isolated/${name}` specified above

* @example () => import(/* webpackChunkName: "isolated/Moon" * /, "@/components/Moon")

*/

lazy: LazyFactory<T>;

Also this:

pixiebrix-extension/src/components/IsolatedComponent.tsx

Lines 108 to 112 in 1334a91

if (!isolatedComponentList.some((url) => url.endsWith("/" + name))) {

throw new Error(

`Isolated component "${name}" is not listed in isolatedComponentList.mjs. Add it there and restart webpack to create it.`,

);

}

…mponents

BLoe · 2024-04-08T17:51:10Z

jest.config.js

+    "\\.(gif|svg|png)$": "<rootDir>/src/__mocks__/stringMock.js",
+
+    "\\?loadAsUrl$": "<rootDir>/src/__mocks__/stringMock.js",
+    "\\?loadAsText$": "<rootDir>/src/__mocks__/stringMock.js",


Does this conflict with the loadAsText one on line 109 below?

Given that tests and snapshots don't need to be updated, I'm guessing that it's not actually being applied nor needed.

Anyway I think these 4 above should be moved below.

I just wondered if it's related to the changes to imports in IntegrationConfigEditorModal.test.tsx

Sort of. I dropped it from that test file because:

?loadAsText did not work in that file, as the original comment suggested, so having it there did nothing

this new config made that import etc?loadAsText point to the stringMock file instead of the actual yml file

Since ?loadAsText cannot "load the import as text" in Jest, it cannot be used if your tests depend on the content of the file.

I cleaned up line 109 as well: 1334a91

src/components/IsolatedComponent.tsx

src/components/integrations/IntegrationConfigEditorModal.test.tsx

…mponents

github-actions · 2024-04-10T12:27:43Z

No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack.

Do not edit this comment manually.

fregante added 9 commits April 4, 2024 18:11

POC: Isolated components

46d2207

Wrap SelectionMenu

708ba2b

Extract CSS, use regular element for now

0671574

Wrap PropertyTree

386b02c

Wrap SelectionToolPopover

6a273f9

Fix tests

360f843

Fix unrelated mutation

e3018e8

https://togithub.com/sindresorhus/eslint-plugin-unicorn/issues/1514 https://togithub.com/pixiebrix/pixiebrix-extension/pull/4486

Update docs

4760ee0

Cleanup

a2c75b9

fregante added infrastructure developer experience labels Apr 4, 2024

fregante commented Apr 4, 2024

View reviewed changes

fregante added 3 commits April 4, 2024 22:07

Some components need no style; test Stylesheets component

9760393

Ok but no fonts?

df1ec00

Ok with fonts

a6dc0fe

vercel bot deployed to Preview April 6, 2024 09:52 View deployment

Import issues

652339a

vercel bot deployed to Preview April 6, 2024 10:04 View deployment

fregante mentioned this pull request Apr 6, 2024

Bump node to 20.12 #8175

Merged

Lint

c5aa5f6

fregante added 3 commits April 6, 2024 17:37

Move init to <IsolatedComponent>

3d139a2

Fix most issues

123b53a

Merge branch 'main' into F/feature/isolated-components

16542a2

vercel bot deployed to Preview April 6, 2024 11:17 View deployment

Extract/cleanup

fb4d8a4

vercel bot deployed to Preview April 6, 2024 11:50 View deployment

/2

a349a66

fregante commented Apr 6, 2024

View reviewed changes

fregante changed the title ~~WIP: #7670: Simplify creation of isolated widgets (IsolatedWidget)~~ #7670: Simplify creation of isolated widgets (IsolatedWidget) Apr 6, 2024

Update documentation

f195c5c

vercel bot deployed to Preview April 8, 2024 13:07 View deployment

fregante mentioned this pull request Apr 8, 2024

#7670: Use <IsolatedComponent> for DocumentView, EphemeralFormContent, CustomFormComponent #8200

Merged

7 tasks

fregante added a commit that referenced this pull request Apr 8, 2024

Minor code changes extracted from #8151 (#8176)

1995795

Merge remote-tracking branch 'origin/main' into F/feature/isolated-co…

fee63aa

…mponents

vercel bot deployed to Preview April 8, 2024 15:40 View deployment

Bad merge

e29f974

vercel bot deployed to Preview April 8, 2024 15:59 View deployment

grahamlangford requested review from BLoe and mnholtz April 8, 2024 17:10

BLoe reviewed Apr 8, 2024

View reviewed changes

src/components/IsolatedComponent.tsx Show resolved Hide resolved

BLoe reviewed Apr 8, 2024

View reviewed changes

src/components/IsolatedComponent.tsx Outdated Show resolved Hide resolved

Lint

82b3dd4

BLoe reviewed Apr 8, 2024

View reviewed changes

src/components/IsolatedComponent.tsx Outdated Show resolved Hide resolved

BLoe reviewed Apr 8, 2024

View reviewed changes

src/components/integrations/IntegrationConfigEditorModal.test.tsx Show resolved Hide resolved

vercel bot deployed to Preview April 8, 2024 19:38 View deployment

fregante mentioned this pull request Apr 10, 2024

MS Edge: clicking a link in a sidebar mod crashes MS Edge #8206

Closed

Merge remote-tracking branch 'origin/main' into F/feature/isolated-co…

3313bc3

…mponents

vercel bot deployed to Preview April 10, 2024 10:14 View deployment

Sort jest config

1334a91

vercel bot deployed to Preview April 10, 2024 10:58 View deployment

grahamlangford approved these changes Apr 10, 2024

View reviewed changes

fregante merged commit 5a4565c into main Apr 10, 2024
28 of 29 checks passed

fregante deleted the F/feature/isolated-components branch April 10, 2024 12:51

fregante restored the F/feature/isolated-components branch April 10, 2024 15:16

fregante deleted the F/feature/isolated-components branch April 10, 2024 15:39

fregante mentioned this pull request Apr 10, 2024

#7670: Ensure stylesheets are removed in IsolatedComponent #8213

Merged

2 tasks

grahamlangford added this to the 1.8.13 milestone Apr 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#7670: Simplify creation of isolated widgets (`IsolatedComponent`) #8151

#7670: Simplify creation of isolated widgets (`IsolatedComponent`) #8151

fregante commented Apr 4, 2024 •

edited

fregante Apr 4, 2024

fregante Apr 4, 2024

fregante Apr 4, 2024

fregante Apr 4, 2024

fregante Apr 4, 2024

fregante Apr 4, 2024

fregante Apr 4, 2024

fregante Apr 4, 2024

codecov bot commented Apr 6, 2024 •

edited

fregante Apr 6, 2024

fregante Apr 6, 2024

mnholtz Apr 8, 2024 •

edited

fregante Apr 10, 2024

fregante Apr 10, 2024

BLoe Apr 8, 2024

fregante Apr 8, 2024

BLoe Apr 8, 2024

fregante Apr 10, 2024

github-actions bot commented Apr 10, 2024

		const ShadowRoot = EmotionShadowRoot.div!;
		// TODO: Use EmotionShadowRoot["pixiebrix-widget"] to avoid any CSS conflicts. Requires snapshot/test updates

	/**
	* It must match the `import()`ed component's filename
	*/
	name: string;

	/**
	* It must follow the format `isolated/${name}` specified above
	* @example () => import(/* webpackChunkName: "isolated/Moon" * /, "@/components/Moon")
	*/
	lazy: LazyFactory<T>;

	if (!isolatedComponentList.some((url) => url.endsWith("/" + name))) {
	throw new Error(
	`Isolated component "${name}" is not listed in isolatedComponentList.mjs. Add it there and restart webpack to create it.`,
	);
	}

#7670: Simplify creation of isolated widgets (IsolatedComponent) #8151

#7670: Simplify creation of isolated widgets (IsolatedComponent) #8151

Conversation

fregante commented Apr 4, 2024 • edited

What does this PR do?

Future work

Checklist

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

codecov bot commented Apr 6, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnholtz Apr 8, 2024 • edited

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

github-actions bot commented Apr 10, 2024

#7670: Simplify creation of isolated widgets (`IsolatedComponent`) #8151

#7670: Simplify creation of isolated widgets (`IsolatedComponent`) #8151

fregante commented Apr 4, 2024 •

edited

codecov bot commented Apr 6, 2024 •

edited

mnholtz Apr 8, 2024 •

edited