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

support remove styles of injectGlobal #2131

Open
zhaoyao91 opened this issue Nov 23, 2020 · 11 comments
Open

support remove styles of injectGlobal #2131

zhaoyao91 opened this issue Nov 23, 2020 · 11 comments

Comments

@zhaoyao91
Copy link

The problem

I am using emotion in sevelte components, where I can't use @emotion/react, and instead I could use @emotion/css.

I want to inject some global styles when component mount and remove them when component unmount. With injectGlobal I have no way to undo the injection.

Proposed solution

Alternative solutions

Additional context

  • @emotion/css: 11.0.0
@Andarist
Copy link
Member

This is an interesting use case but it's also quite problematic to handle right now and supporting this out of the box in the current injectGlobal could have been a breaking change.

You can implement your own version of injectGlobal under 20 LOC that would support this:

import { serializeStyles } from "@emotion/serialize";
import { StyleSheet } from "@emotion/sheet";
import { serialize, compile, middleware, rulesheet, stringify } from "stylis";

function injectGlobal(...args) {
  const { name, styles } = serializeStyles(...args);
  const sheet = new StyleSheet({
    key: `global-${name}`,
    container: document.head
  });
  const stylis = (styles) =>
    serialize(
      compile(styles),
      middleware([
        stringify,
        rulesheet((rule) => {
          sheet.insert(rule);
        })
      ])
    );
  stylis(styles);
  return () => sheet.flush();
}

You can check out the demo here. It's using React for rendering as that was just convenient for me but doesn't use any React-specific APIs for styling.

@zhaoyao91
Copy link
Author

@Andarist thanks for your impressive workaround. I will use it in the app, and also hope such a feature to be integrated into the @emotion/css since it will make a closed loop.

@deleteme
Copy link

Coincidentally, I was also looking for a way to clean up after injectGlobal yesterday. Returning a teardown function is elegant and conventional. Is there any chance we could see this API change in a future release?

@Andarist
Copy link
Member

This is unlikely to be implemented in v11 because, as you might see, my solution creates a new StyleSheet for those global styles and that would be a breaking change for v11.

I'm going to keep it open so we can take a look at this before releasing v12 (which probably won't happen any time soon as we have just released v11).

@gregjacobs
Copy link

As an aside on this: any plans to support general rule removal in the future? And, is there a case for this? (Maybe for mobile browsers to clean up memory and improve selector performance?)

If so, could leverage the same mechanism to return a teardown function from injectGlobal() which only removes the rules that it injected, but leaves the rest of the rules inside the <style> element alone

@Andarist
Copy link
Member

As an aside on this: any plans to support general rule removal in the future? And, is there a case for this? (Maybe for mobile browsers to clean up memory and improve selector performance?)

We haven't found any real perf implications for having rules inserted in the CSSOM throughout the entire lifetime of the application.

If so, could leverage the same mechanism to return a teardown function from injectGlobal() which only removes the rules that it injected, but leaves the rest of the rules inside the <style> element alone

Yes, that would certainly be in general doable but given that you actually can insert multiple rules using a single call and yet they are inserted to a single <style> this would just increase the complexity in a way that I think it's not worth it right now.

@gregjacobs
Copy link

@Andarist Interesting. Ok, thanks for that, and the quick response. Removal of rules has been on the back of my mind for large portal systems which load many apps as the user is using it, but maybe it's not necessary after all.

Best,
Greg

@alexmorleyfinch
Copy link

Another use-case for removing global styles is:

When injecting a plugin into a webpage via a browser extension, and then completely removing that plugin upon closing/removal.

If we don't clean up on close, then on re-open we get warnings in the console: "You are loading @emotion/react when it is already loaded"

Unfortunately we use a framework that uses emotion, so we cannot use this work around so easily. We'll just have to pollute the DOM and accept the warning instead.

I find it silly to not have provided a way to clean up after yourself, just because there wasn't an obvious use-case for it... In my opinion this was a big oversight. You should always provide a method for clean up. It's just common sense.

@Andarist
Copy link
Member

If we don't clean up on close, then on re-open we get warnings in the console: "You are loading @emotion/react when it is already loaded"

This likely isn't strictly related to injectGlobal. You can end up with this warning even if not using this API. injectGlobal is also a thing coming from @emotion/css package but you are mentioning @emotion/react.

I find it silly to not have provided a way to clean up after yourself, just because there wasn't an obvious use-case for it... In my opinion this was a big oversight. You should always provide a method for clean up. It's just common sense.

You can always clean up the whole vanilla Emotion instance with flush. We just don't support removing individual global rules at the moment.

@gregjacobs
Copy link

gregjacobs commented May 30, 2023

I find it silly to not have provided a way to clean up after yourself, just because there wasn't an obvious use-case for it... In my opinion this was a big oversight.

@alexmorleyfinch I'm not the library author, but thinking more about the implementation, it's not exactly straightforward to implement the removal of individual style rules from a stylesheet. This is especially true if you want to make it performant and memory efficient (i.e. something better than O(n*m) time and O(n) memory, where n is the number of style rules in the sheet, and m is the number of rules you want to remove).

Since the only way to remove individual style rules from a stylesheet is by their numeric index, even the way to make it efficient still involves spending memory on some sort of bookkeeping data structure which stores the ranges of indices to remove for each injectGlobal set of styles. The index ranges in this data structure would also need to be updated when style rules are removed because the indices "shift down" for rules that appear later in the sheet.

Worth it to implement? Maybe. Should probably be an opt-in feature if so though, so that not all users need to take with the performance/memory cost of maintaining this structure if they don't need it.

As far as your implementation goes: why not just create a new Emotion instance with a unique key for your browser extension, which would then allow you to call .flush() on it when you want your styles removed?

@alexmorleyfinch
Copy link

Thanks for the clarifications @Andarist @gregjacobs, much appreciated 👍

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

No branches or pull requests

5 participants