feat: Allow providing custom StyleSheetRegistry (fix async render issues)

[twavv]

Should fix #64 (while trying to keep existing workflows working and changing as few lines of code as possible).

Changes

• Introduce StyleSheetRegistryContext (whose default value is globalStyleSheetRegistry -- means should work without changes for existing users who don't care about async rendering)
• Bump React requirement to 16.3 to get new context. (we could get around this if people feel strongly, but... React 16.3 should be a decent minimum in my oh so humble opinion)

---

This is still somewhat of a WIP, but I think it's at the point that it could be review.

[twavv]

This does seem to work in my NextJS codebase.

// _document.tsx (abridged)

class AppDocument extends Document {
  static async getInitialProps(ctx: DocumentContext) {
    // create a per-request registry
    const styledJsxRegistry = new StyleSheetRegistry();

    // override renderPage (as suggested in NextJS documentation)
    const originalRenderPage = ctx.renderPage;
    ctx.renderPage = (opts) => {
      return originalRenderPage({
        ...opts,
        enhanceApp: (App) => (props) => (
          <StyleSheetRegistryContext.Provider value={styledJsxRegistry}>
            <App {...props} />
          </StyleSheetRegistryContext.Provider>
        ),
      });
    };

    const initialProps = await Document.getInitialProps(ctx);
    const styledJsxStyles = flushToReact({ registry: styledJsxRegistry });

    return {
      ...initialProps,
      styles: (
        <>
          {initialProps.styles}
          {styledJsxStyles}
        </>
      ),
    };
  }

  ...
}

[giuseppeg]

Hi @travigd thanks for the PR!! So far so good! Can you add some tests? @ me when the PR is ready.

A few comments:

[giuseppeg]

Unfortunately the constructor of StyleSheetRegistry creates a new StyleSheet instance and injects it into the page. This would be useless when you don't use the global registry/stylesheet.

Perhaps we can create a global and shared StyleSheetRegistry in style.js only when this.context is undefined?

[twavv]

Two things:

First thing:
The predominant use case for this will be server rendering, where the sheet isn't injected (since it's on the server). When client rendering, we can just use the global instance.

Second thing:
Are you sure about that assessment? Either way, the StyleSheet gets injected when StyleSheetRegistry is constructed. If that happens only during SSR, it's a no-op, but if you're using StyleSheetRegistryContext as part of CSR, you'll still presumably construct a new StyleSheetRegistry and it will be injected into the DOM.

[giuseppeg]

Either way, the StyleSheet gets injected when StyleSheetRegistry is constructed.

Correct

but if you're using StyleSheetRegistryContext as part of CSR

If you used multiple StyleSheetRegistry each using a dedicated StyleSheet, you'll have the same amount of StyleSheets on the client (due to hydration the same code that ran on the server will run on the client).

What I meant with my previous comment is that if you don't use the globalStyleSheetRegistry instance, on the client you'll have one extra (unused) <style> element.

Probably not a big deal and the following is not worth and ugly, but the above could be avoided if in style.js you do the following:

let globalStyleSheetRegistry = null
export default class JSXStyle extends Component {
  // ... 
  get styleSheetRegistry() {
      if (this.context instanceof StyleSheetRegistry) { 
          return this.context
      }
      
       if (!globalStyleSheetRegistry) {
              globalStyleSheetRegistry = new StyleSheetRegistry()
       }
       return globalStyleSheetRegistry
   }
}

with

export const StyleSheetRegistryContext = React.createContext(null)

[twavv]

Oh, I see what you mean.

Personally, I think having the global registry just always present is fine even if it's empty (especially since I suspect the predominant use case will only be wrapping it for a SSR then using the global registry for everything on the client). My NextJS example that I added doesn't suffer from this for exactly that reason (the _document code only runs on the server).

If you'd really like me to change something, let me know I can do it. But like I said, I think the current behavior in this PR is fine.

[twavv]

@giuseppeg I'm happy with this PR.

If we really want to preserve compat with React <16.3, we can probably hack it in (with conditional if (typeof React.createContext !== undefined) statements). But React 16.3 was released almost three years ago.

EDIT: Prettier decided to update the entire README as part of the commit hook, but the only thing that I changed was the section I added about asynchronous rendering.

[giuseppeg]

Is there any reason for not passing a cache here? Probably it is ok to export a version with a built in cache (the iife) I wrote in the other comment.

[twavv]

I can do that. My only worry is just that it might become a bit of a memory leak since you have a cache entry per unique set of props (so if you have a long running app that uses different interpolations, they'd never get cleared).

What do you think?

[giuseppeg]

Oh well it has been like this forever and nobody has reported any issue of this kind. If necessary we can replace the object with a WeakMap any time though (later)

[giuseppeg]

I'd say let's fix this (bring back the cache)

[twavv]

Hey @giuseppeg , just bumping this. No huge rush but would love to get this merged and released. 🎉

[giuseppeg]

Hi @travigd sorry for making you wait – I haven't had time to check the PR again but it looks good, there are just a couple of unresolved comments (especially the one about the cache) that might be worth fixing.

@timneutkens can you take a look at this PR when you have time? I just want to make sure that these changes won't conflict with future plans of yours (assuming styled-jsx will be updated to support concurrent mode or server components when they are out).

[twavv]

Yep! Thank you! I did respond to the caching thing as being a potential memory leak if we use a global cache that is never flushed during SSR. :)

[twavv]

ping @timneutkens

[twavv]

@timneutkens @giuseppeg Any way we can move forward with this? :)

[timneutkens]

Overall this looks good, however I'm not convinced we should provide backwards compat with the previous approach, it'll likely be a lot simpler to release a new major version that does not support the global registry which is an anti-pattern nowadays anyway 👍

The version in Next.js is pinned so that would not affect existing apps.

[twavv]

If I get a say, I'd suggest releasing this as a minor then re-write the code base to include a few breaking changes (like this) and also adopt typescript and maybe reduce reliance on snapshot testing while we're at it.

[timneutkens]

Overall there's no benefit in releasing a minor though as styled-jsx is mostly used with Next.js.

[twavv]

Maybe I'm confused, but... I was able to install this version onto my NextJS project and it works just fine.

[giuseppeg]

This issue #714 made me realize that if we want to support multiple registries we'd need to find a way to hydrate SSR styles.

Currently the first instance of the stylesheet registry will handle those styles

styled-jsx/src/stylesheet-registry.js

Lines 202 to 205 in 2741879

	selectFromServer() {
	const elements = Array.prototype.slice.call(
	document.querySelectorAll('[id^="__jsx-"]')
	)

[huozhi]

We landed a new context based API in v5 beta. Please checkout the readme for the information. Thanks for the PR!