Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom root & shadow DOM support (#832) #1004

Open
wants to merge 1 commit into
base: canary
Choose a base branch
from

Conversation

jlalmes
Copy link

@jlalmes jlalmes commented Apr 13, 2022

Closes #832.

root was incorrectly removed from config here: #836

Custom root & shadow DOM support

The root config property is causing a TypeError - Converting circular structure to JSON error. The root cause of this is the createMemo function (introduced here) attempting to JSON.stringify circular objects such as document.

Changes made by this PR

  • Added better stringify function used by createMemo.js (heavily inspired by safe-json-stringify)
  • Added root to CreateStitches & ConfigType TS definitions (for core & react).
  • Added new regression test for #832

👇 Web Components & shadow DOM demo

https://codesandbox.io/s/circular-json-reproduction-forked-gy01cx?file=/src/index.tsx

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 889c04c:

Sandbox Source
Stitches CI: CRA Configuration
Stitches CI: Next.js Configuration
Circular JSON reproduction Issue #832

@jlalmes
Copy link
Author

jlalmes commented Apr 13, 2022

An alternate solution might be removing the stringify in createMemo.

export const createMemo = () => {
    const cache = new Map()
    
    return (value, apply, ...args) => {
        if (cache.has(value)) {
            return cache.get(value)
        }
        const result = apply(value, ...args)
        cache.set(value, result)
        return result
    }
}

@jonathantneal could you give some feedback on this?

@jlalmes jlalmes changed the title Add root to config (#832) Custom root & shadow DOM support (#832) Apr 13, 2022
@aMediocreDad
Copy link

If it is of interest I wrote a different solution last year (it was for use in a personal project as it uses the constructed stylesheets polyfill). aMediocreDad@14660d4

@cinchy-m
Copy link

@jlalmes Any chance you could add core lib usage instructions for web components to the docs in this PR?

@jlalmes
Copy link
Author

jlalmes commented Apr 24, 2022

Happy to update the Stitches website docs once this PR is merged 👍

@LexSwed
Copy link

LexSwed commented Jun 5, 2022

Hey @hadihallak 👋
Any chance stitches usage with Shadow DOM can be included in the next Milestone? The issue I'm looking to solve is the ability to pass the root to the createStitches config, so that I do not polute user page styles with stitches properties when developing browser extension with a content script that renders UI in own shadow root.

@hadihallak hadihallak self-requested a review June 5, 2022 23:45
@hadihallak
Copy link
Member

@jlalmes Thanks for your hard work.. this is something that's under consideration for the next release.
I will be reviewing this in the next couple of days but giving this a quick look it looks good but I'm worried about the added
bundle size so I'm wondering if we can do it some other / more efficient way.

@LexSwed Thanks for the ping
Did you get the chance to test this PR in your setup? have you had any issues?

@LexSwed
Copy link

LexSwed commented Jun 7, 2022

Did you get the chance to test this PR in your setup? have you had any issues?

No, I haven't tried this solution specifically, but this one: #628 (comment) worked in my case

@jlalmes
Copy link
Author

jlalmes commented Jun 7, 2022

Hi @hadihallak - I suggest using a better memoizing function like the one I have mentioned above if bundle size is a concern (#1004 (comment)). Map browser support is all green https://caniuse.com/mdn-javascript_builtins_map.

Let me know what you think and I will update the PR.

@hadihallak
Copy link
Member

@jlalmes @LexSwed I think we're gonna delay merging this until after the 1.3 release as even with root support via the config, this doesn't solve providing root at runtime like in other libraries so I'm gonna think about this after the 1.3 release and come up with a plan to include runtime root support or work on merging this with the same old root API that exists here.

Just to be extra clear, we want to support this as it's a much needed feature even for things not mentioned in this PR but we don't want to be hasty with API decisions (I know this used to exist prior to v1 but it wasn't the ideal solution).

Thank you so much for your hard work 🙏

@IsLand-x
Copy link

Need shadow DOM feature support.

When creating chrome extension and injecting ui into content script, this "css in js" library will be the best practice if shadow dom feature was supported.

@precious-void
Copy link

Any updates on the status of PR?

@he-la
Copy link

he-la commented Mar 23, 2023

Don't we also have to add the root type to the exported config type in stitches.d.ts?

From: Henrik Laxhuber <[email protected]>
Date: Wed, 22 Mar 2023 20:09:29 +0100
Subject: [PATCH] Shadow DOM Support

---
 packages/core/types/stitches.d.ts  | 4 +++-
 packages/react/types/stitches.d.ts | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/packages/core/types/stitches.d.ts b/packages/core/types/stitches.d.ts
index 42a7200..3ed6636 100644
--- a/packages/core/types/stitches.d.ts
+++ b/packages/core/types/stitches.d.ts
@@ -12,7 +12,8 @@ export default interface Stitches<
 	Media extends {} = {},
 	Theme extends {} = {},
 	ThemeMap extends {} = {},
-	Utils extends {} = {}
+	Utils extends {} = {},
+	Root extends DocumentOrShadowRoot = Document
 > {
 	config: {
 		prefix: Prefix
@@ -20,6 +21,7 @@ export default interface Stitches<
 		theme: Theme
 		themeMap: ThemeMap
 		utils: Utils
+		root?: Root
 	},
 	prefix: Prefix
 	/** The **prefix** property defined.
diff --git a/packages/react/types/stitches.d.ts b/packages/react/types/stitches.d.ts
index 846f281..9d7b986 100644
--- a/packages/react/types/stitches.d.ts
+++ b/packages/react/types/stitches.d.ts
@@ -12,7 +12,8 @@ export default interface Stitches<
 	Media extends {} = {},
 	Theme extends {} = {},
 	ThemeMap extends {} = {},
-	Utils extends {} = {}
+	Utils extends {} = {},
+	Root extends DocumentOrShadowRoot = Document
 > {
 	config: {
 		prefix: Prefix
@@ -20,6 +21,7 @@ export default interface Stitches<
 		theme: Theme
 		themeMap: ThemeMap
 		utils: Utils
+		root?: Root
 	},
 	prefix: Prefix
 	/** The **prefix** property defined.
--
2.38.3

@he-la
Copy link

he-la commented Mar 23, 2023

While at it, might be useful to also support multiple roots + changing roots dynamically? If you have a need for Shadow DOM, then chances are you'll have more than one Shadow DOM.

@maylynn-ng
Copy link

Hii!! Running into this issue, this would be an awesome addition! Any updates? 🙏

@maylynn-ng
Copy link

While at it, might be useful to also support multiple roots + changing roots dynamically? If you have a need for Shadow DOM, then chances are you'll have more than one Shadow DOM.

If this PR is looking good for a single one, we could maybe do that as a follow up PR to unblock some use cases? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stitches throwing exceptions "TypeError - Converting circular structure to JSON"
9 participants