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 StyleSheet #2675

Closed
Jack-Works opened this issue Mar 7, 2022 · 15 comments · Fixed by #2677
Closed

Custom StyleSheet #2675

Jack-Works opened this issue Mar 7, 2022 · 15 comments · Fixed by #2677

Comments

@Jack-Works
Copy link
Contributor

let sheet = new StyleSheet({

Hi! I found emotion in creating a new Sheet based on the old sheet. Is it possible to get rid of this behavior, or use the same constructor as the cache.sheet has?

I'm handling a very special environment and I must implement my own StyleSheet class to insert rules. It's be like this:

    const emotionCache = createEmotionCache({ key: keyA })
    const styleSheet = new MySpecialStyleSheet(keyA, shadow)
    emotionCache.sheet = styleSheet

It works well besides the global styles. Global styles using the emotion's original implementation therefore I cannot control the insertRule behavior.

If I can provide my own StyleSheet implementation, or at least emotion can write this to use my StyleSheet:

let sheet = new cache.sheet.constructor({
    ...
})

it will be great! thanks!

@Andarist
Copy link
Member

Andarist commented Mar 7, 2022

Hi there,

nice to see you here - thank you for your great OSS work! I can't wait for throw types to happen 😉

Using cache.sheet.constructor within Global seems like something that we could do. Considering it's a simple change and that it wouldn't break anything and doesn't really introduce any new options/configuration etc this is a solution that I would prefer. Especially that you could only tweak your cache once and the rest of the thing would "just work".

If you prepare a PR for such a change with tests then I would happily merge it.

I'm handling a very special environment and I must implement my own StyleSheet class to insert rules.

Out of my curiosity - could you describe this in more detail? I love learning about quirky use cases :P

@Jack-Works
Copy link
Contributor Author

nice to see you here - thank you for your great OSS work! I can't wait for throw types to happen 😉

thanks! but it's likely not to happen because the TypeScript team doesn't approve that feature.

Out of my curiosity - could you describe this in more detail? I love learning about quirky use cases :P

Our application is rendered in the Shadow Root.

When a Dialog is opened, it needs to be rendered into another ShadowRoot (in order to have the correct DOM hierarchy to be the frontest DOM). Therefore, our application actually needs to render the same StyleSheet in multiple places.

As you can see, this component sits inside a ShadowRoot.

image

And this dialog is in the out-most DOM to show in the top-most.

image

We wrote code in this way:

const [open, setOpen] = useState(false)
return <>
    // This is rendered as the screenshot 1
    <Button onClick={() => setOpen(true)}><Icon /></Button>
    // This dialog is rendered as the screenshot 2
    {useShadowRootPortal(container => <ComposeDialog open={open} ModalProps={{ container }} />)}
</>

So we need to clone styles between different ShadowRoots.

My implementation of StyleSheet for our environment is available at ShadowRootStyleSheet.ts

On Chrome, we choose to use Constructable StyleSheet to share CSS between different ShadowRoots, on other platforms, we choose to clone those styles manually when sheet.insertRule is called.

Currently, I provided a fake sheet.container that has insertBefore monkey-patched so I can know when there is a global style inserted.

this.container.insertBefore = (child) => {
    if (child instanceof HTMLStyleElement) {
        child.appendChild = (child) => {
            if (child instanceof Text) this.implementation.insertGlobal(child.wholeText)
            return child
        }
    }
    return child
}

If you prepare a PR for such a change with tests then I would happily merge it.

Thanks! I need to consider if emotion changes to cache.sheet.constructor, how I should change the code to join in (we have an incompatible new signature for our version of StyleSheet now).

@Jack-Works
Copy link
Contributor Author

I have some other questions about emotion 👀👀

  • Why cannot Global styles use the same StyleSheet? Is it some problem with the CSS order?
  • In StyleSheet, there is only a method called insertRule. What if emotion needs to delete a rule no longer needed?

@Jack-Works
Copy link
Contributor Author

I've tried to patch emotion on my project to test if new cache.sheet.constructor could help us, I found our current monkey patch is the simplest solution so I think no change is needed on emotion. Thanks for your help!

@Andarist
Copy link
Member

Andarist commented Mar 8, 2022

Why cannot Global styles use the same StyleSheet? Is it some problem with the CSS order?

In a way - yes. We insert global styled before other styles from the "main" sheet. So yes, changing this could result in different CSS being applied on a page because of how cascade works. Note though that if you have multiple Global styles on a page then their order is not guaranteed - all of them should be inserted before the "main" sheet but it's not obvious which global styles will take precedence over other global styles etc.

In StyleSheet, there is only a method called insertRule. What if emotion needs to delete a rule no longer needed?

We just over-cache stuff - we do not implement any kind of ref-counting etc to clean "unused" styles. Theoretically, those styles might become needed again and in such a case we won't have to reinsert them, we'll just reuse what has already been inserted. Also, note that removing rules has a cost for the browser - so it's not like we are facing an obvious choice here that removing styles is better for performance, it could actually be worse.

This ain't 100% true for Global styles because those are removable - and it's also a partial reason why those are inserted into another sheet. This way we can just call globalSheet.flush() when the Global component unmounts (or changes its styles) without affecting the "main" sheet at all.

I've tried to patch emotion on my project to test if new cache.sheet.constructor could help us, I found our current monkey patch is the simplest solution so I think no change is needed on emotion.

So patching this didn't help you? I think this kinda would be a nice feature to add nevertheless - so I might introduce such a change later on my own.

@Jack-Works
Copy link
Contributor Author

This way we can just call globalSheet.flush() when the Global component unmounts (or changes its styles) without affecting the "main" sheet at all.

Oh, I forgot global styles can be removed... So our patch currently doesn't work correct for this case, because calling globalSheet.flush() doesn't flush on my ShadowRootSheet.

So patching this didn't help you?

I patched local files and found it will make our code more complex.

If it's possible to not create a new StyleSheet, but change the shape of StyleSheet, to add the following methods:

insertGlobal(rule: string): void
flushGlobal(): void

It will be cleaner on our sides. I can implement those two methods to get the correct behavior without patching insertBefore and appendChild.

@Andarist
Copy link
Member

Andarist commented Mar 8, 2022

This gets slightly more complicated because each Global needs its own flushable "slot". So if anything this would have to be implemented:

// returns DisposeGlobal
insertGlobal(rule: string): () => void

But then it gets harder to actually call flush (or flushGlobal) from within Global because insertStyles that is used within it doesn't return anything. We obviously could introduce more code there to return those dispose functions (cause actually sheet.insert might be called multiple times within a single insertStyles call) somehow etc but that's quite a bit of code to accommodate quite a niche use case that you have there (considering that this didn't come up until now).

So if possible I would really prefer to keep the originally proposed solution of:

let sheet = new cache.sheet.constructor({
    ...
})

I understand that this might complicate stuff a little bit for you but on the other hand, this wouldn't really spill into your app code, the complexity would just be contained to the implemented cache wrapper. So I think it's a reasonable tradeoff here.

Could you share a git patch (git diff) introducing changes to your code required to make the originally proposed solution to work?

@Jack-Works
Copy link
Contributor Author

Could you share a git patch (git diff) introducing changes to your code required to make the originally proposed solution to work?

Umm actually I didn't complete my try on this route because I stopped when I found I already modified too much things.

What about this? Add a sheet.newGlobalSheet() method, the emotion implementation will be return new StyleSheet({ key: this.key + '-global', ... }) and my implementation will be

newGlobalSheet() {
    return {
        insert(x) { this.implementation.insertGlobal(x) }
        flush() { this.implementation.flushGlobal }
    }
}

Then change let sheet = new StyleSheet(...) to let sheet = sheet.newGlobalSheet().

This will make implementation on my side much simpler cause I don't need to distinguish if the constructor call is by my own code or from emotion for the global styles.

@Andarist
Copy link
Member

Andarist commented Mar 8, 2022

Umm actually I didn't complete my try on this route because I stopped when I found I already modified too much things.

Could you summarize where complexity was? I'd like to understand the problem space really well here before moving towards other solutions.

I somewhat don't see why the latest proposal with newGlobalSheet would make a big difference because when patching in a code like this:

    const emotionCache = createEmotionCache({ key: keyA })
    const styleSheet = new MySpecialStyleSheet(keyA, shadow)
    emotionCache.sheet = styleSheet

You can always assume that all calls to new cache.sheet.constructor will happen from within the Global component. So I wonder why you can't just replace this constructor here with whatever implementation you want. You could even close over some extra arguments here etc (as I assume that they stay the same for the same instance of the cache). I know that this might require some JS gymnastics but if you can write this:

    class MySpecialStyleSheet {
      // ...
      newGlobalSheet() { /* ... */ }
    }
    const emotionCache = createEmotionCache({ key: keyA })
    const styleSheet = new MySpecialStyleSheet(keyA, shadow)
    emotionCache.sheet = styleSheet

Then you almost always should be able to write something here to make the MySpecialStyleSheet.prototype.constructor returning a sheet that would behave in a special way for the global case.

@Andarist
Copy link
Member

Andarist commented Mar 8, 2022

And note that I'm not saying a definitive no to expanding the current implementation etc. For instance, I've always wanted to make those vanilla~ global styles flushable:

I would just like to understand your PoV better first - figure out a solution for that and then perhaps move to figure out this other problem. Maybe they are strongly coupled and related and we must solve both at the same time but I don't see that yet.

@Jack-Works
Copy link
Contributor Author

Why I propose the newGlobalSheet is because I need to know the original sheet instance when the global version is constructed.

As you can see,

newGlobalSheet() {
    return {
        insert(x) { this.implementation.insertGlobal(x) }
        flush() { this.implementation.flushGlobal }
    }
}

in this example, this refers to the normal sheet I created.

If we change to cache.sheet.constructor, it is still possible to get the original sheet.

Example:

// @ts-ignore
this.container.sheet = this

And then I can read options.container.sheet back, but that is less ideal and more like a hack.

I think any of those two solutions are cool but I prefer the newGlobalSheet because I do not need to hack anymore.

@Jack-Works Jack-Works reopened this Mar 8, 2022
@Andarist
Copy link
Member

Andarist commented Mar 8, 2022

Could you prepare pseudocode of what you have there? Then I perhaps could propose a way to do this using the cache.sheet.constructor solution (although you are probably capable of figuring this out on your own, I just maybe would have some idea how to make it simpler).

I know that this might require some "hacks" but ultimately I don't think it will be that bad (happy to be proven wrong though). I would just like to first unblock you, even with a somewhat hacky solution, and only then take our time to improve the design that maybe would make it even simpler for you (I'm not committing to it just yet though, cause I'd like to really evaluate our possibilities here and I have a lot on my plate lately).

@Jack-Works
Copy link
Contributor Author

thanks! it's no longer blocks us, our new implementation has already landed. it is based on the hack of container.insertBefore

If emotion chooses to cache.sheet.constructor, we will change the code to:

class MySheet {
    container = document.createElement('div')
    constructor(options) {
        if (options.container.sheet) {
            return new MyGlobalSheet(this)
        } else {
            this.container.sheet = this
            return new ConstructableSheet(this)
        }
    }
}

@Jack-Works
Copy link
Contributor Author

If emotion choose newGlobalSheet, it will be:

class ConstructableSheet {
    constructor(options) {
        // ...
    }
    newGlobalSheet() {
        return new GlobalSheet(this)
    }
}

This one is simpler on our side and has better semantics, but the cache.sheet.constructor way is also good.

@Andarist
Copy link
Member

Andarist commented Mar 9, 2022

Would you be open to preparing a PR for the cache.sheet.constructor change?

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 a pull request may close this issue.

2 participants