Add automatic global style injection #48

wbern · 2020-09-01T21:53:07Z

Fixes #47

marvinhagemeister

Thanks for making a PR 👍 Made some quick comments, need to do a more thorough review after work 🎉

src/index.js

src/index.test.jsx

marvinhagemeister

Finally had the time to go through the PR in detail. It's amazing to see that you went through all scenarios and even added tests (which is rare for PRs)! Love this 🙌

Left a few comments regarding ways to simplify the API with the intention of keeping maintenance costs low as this library evolves. While tinkering with that I found a few ways to cut down on size.

Let me know what you think 👍

marvinhagemeister · 2020-09-03T20:08:59Z

README.md

+
+Attaches a Shadow DOM instance to your Custom Element. Remember that global styles won't affect the Custom Element unless you inject (see `options.injectGlobalStyles`) or somehow import them from the component itself.
+
+#### options.injectGlobalStyles


I wonder if we can slim down the user-facing API surface area. My gut tells me that we can combine the solutions:

options.injectGlobalStyles = true is the same as injectGlobalStyles.selector = '*'

We could eliminate injectGlobalStyles.target by looping over document.styleSheets directly. It includes both inline style tags and sheets added via a link element. Each sheet has a pointer to the node it was created by and we can match against that.

I'd love to start simple and remove filter too. Whilst there are cases in theory where a query selector may not suffice, I can't come up with realistic real world examples where the selector approach falls short. Checking popular CSS-in-JS libs, they all mark their own stylesheets with a special attribute. For CSS-modules the user usually has tight control over naming the assets and we can match on that.

Since there all sheets are inherently global, unless bound to a shadow root, we can drop the Global part of the variable name, imo.

marvinhagemeister · 2020-09-03T20:12:45Z

src/index.js

+			const defaults = {
+				target: document.head,
+				selector:
+					'style, link[rel="stylesheet"], link[rel="preload"][as="style"]',


Strings tend to not compress well. If we switch to document.styleSheets and match our selectors on sheet.ownerNode, we should be able to get rid of it.

marvinhagemeister · 2020-09-03T20:18:55Z

src/index.js

+				observeOptions: { childList: true, subtree: true },
+			};
+
+			this.styleObserver = beginInjectingGlobalStyles(


Super tiny nit: The prefix begin* somewhat implies that there is an end* function somewhere. Seeing that this function is only used once we can inline it and save about 14B.

marvinhagemeister · 2020-09-03T20:24:05Z

src/index.js

+				options.injectGlobalStyles === true
+					? defaults
+					: /* eslint-disable indent */
+					  {


If we do follow through with the stylesheet approach mentioned earlier we can remove this block.

marvinhagemeister · 2020-09-03T20:29:57Z

src/index.js

+		)
+	);
+
+	return observeStyleChanges(


Inlining this function here saves about 23B . It allows us to get rid of the callback function wrapping and additional function call as we can just call cloneElementsToShadowRoot.

marvinhagemeister · 2020-09-03T20:41:19Z

src/index.js

+) {
+	return new MutationObserver((mutations, observer) => {
+		mutations.forEach((mutation) => {
+			const matchedElements = Array.prototype.slice


Worth checking: If we inline cloneElementsToShadowRoot we can loop directly over the NodeList, thereby skipping the additional iteration over elements and saving an array allocation in the process.

marvinhagemeister · 2020-09-03T20:42:18Z

src/index.test.jsx

+		return <span>Hello world!</span>;
+	}
+
+	const styleElem = document.createElement('style');


Love the tests 🙌

wbern · 2020-09-04T05:09:28Z

Wow, good insights! I'll be going through these!

developit · 2020-09-04T18:55:01Z

I'm not entirely sure I understand the need for this to be a feature of preact-custom-element. What's happening here is essentially a variant of something the platform has attempted and removed twice (piercing descendant combinator and <style scoped>). From what I recall at the time when those solutions were removed, it turned out to be extremely difficult to achieve desirable semantics + performance when copying whole stylesheets around. Instead, the current approach is to ratify and ship the new Shadow Parts specification.

My concern as it relates to preact-custom-element is that all current userland solutions have poor performance, since even basic optimizations like structural sharing are not possible. For that reason, I would very much like to avoid promoting anything that relies on these semantics. Additionally, manual stylesheet copying would be extremely difficult to support in combination with platform features like Shadow Parts, Declarative Shadow DOM and Template Instantiation.

As a thought: what about providing a list of stylesheets to inject into the shadow root? This would avoid the implicit copying issues, but still allow for selective style inheritance.

wbern · 2020-09-07T08:26:42Z

@marvinhagemeister your thoughts on above? Before I put in any more work into the PR.

We could just put a callback right after the attachShadow call, so we in the userland can use a solution like this on our own?

Add automatic global style injection

35cf7b8

wbern force-pushed the global-style-injections branch from 79fd43f to 35cf7b8 Compare September 2, 2020 06:23

marvinhagemeister reviewed Sep 2, 2020

View reviewed changes

src/index.js Outdated Show resolved Hide resolved

src/index.js Outdated Show resolved Hide resolved

src/index.test.jsx Outdated Show resolved Hide resolved

src/index.test.jsx Outdated Show resolved Hide resolved

William Bernting (whf962) added 3 commits September 2, 2020 10:32

Remove exports

d7ea02d

Use linkElem.href to match HTML output

4662c0f

Pinpointing the location of indent exception

0d502b8

wbern force-pushed the global-style-injections branch from 85f1c6f to 0d502b8 Compare September 2, 2020 08:33

marvinhagemeister reviewed Sep 3, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add automatic global style injection #48

Add automatic global style injection #48

wbern commented Sep 1, 2020

marvinhagemeister left a comment

marvinhagemeister left a comment

marvinhagemeister Sep 3, 2020

marvinhagemeister Sep 3, 2020

marvinhagemeister Sep 3, 2020

marvinhagemeister Sep 3, 2020

marvinhagemeister Sep 3, 2020

marvinhagemeister Sep 3, 2020

marvinhagemeister Sep 3, 2020

wbern commented Sep 4, 2020

developit commented Sep 4, 2020

wbern commented Sep 7, 2020


		Attaches a Shadow DOM instance to your Custom Element. Remember that global styles won't affect the Custom Element unless you inject (see `options.injectGlobalStyles`) or somehow import them from the component itself.

		#### options.injectGlobalStyles

Add automatic global style injection #48

Are you sure you want to change the base?

Add automatic global style injection #48

Conversation

wbern commented Sep 1, 2020

marvinhagemeister left a comment

Choose a reason for hiding this comment

marvinhagemeister left a comment

Choose a reason for hiding this comment

marvinhagemeister Sep 3, 2020

Choose a reason for hiding this comment

marvinhagemeister Sep 3, 2020

Choose a reason for hiding this comment

marvinhagemeister Sep 3, 2020

Choose a reason for hiding this comment

marvinhagemeister Sep 3, 2020

Choose a reason for hiding this comment

marvinhagemeister Sep 3, 2020

Choose a reason for hiding this comment

marvinhagemeister Sep 3, 2020

Choose a reason for hiding this comment

marvinhagemeister Sep 3, 2020

Choose a reason for hiding this comment

wbern commented Sep 4, 2020

developit commented Sep 4, 2020

wbern commented Sep 7, 2020