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

in WMR prerender: asyncVirtualSheet reset() is not a Promise, yet is awaited #26

Open
danielweck opened this issue Jan 3, 2022 · 7 comments

Comments

@danielweck
Copy link
Member

danielweck commented Jan 3, 2022

await sheet.reset()

...but:

https://github.com/tw-in-js/twind/blob/15441649dcdfb97fba56e0cc8870f08485cfc1c6/src/server/index.ts#L73-L85

reset: () => { /* ... */ }
@danielweck
Copy link
Member Author

Mhmm, this appears to be the cause of bug #25

@rschristian
Copy link
Member

rschristian commented Jan 3, 2022

No, it's not.

So this was my doing, and honestly, was just the result of trial and error.

Without awaiting the reset, when pages that would share styles (say /foo and /bar would be routed to the same content), pages that render afterwards do not contain any styles that exist in the previous. So /bar would be empty (besides the preflight).

Twind utilizes Node's async_hooks, and it seemed to me like there was a bad type somewhere that results in this incorrectly being labeled as sync. AFAIK, awaiting something that's synchronous has no effect.

If there's a better solution I'd love to see it.

Edit: See below for better reproduction of the original issue

@danielweck
Copy link
Member Author

Without awaiting the reset,

but reset() is not an async function / doesn't return a Promise, so how come?

@rschristian
Copy link
Member

rschristian commented Jan 3, 2022

As I said:

just the result of trial and error.

Truly, I don't know, I think it was a misplaced await but it "worked", and seemingly continues to do so.

It very well could be some race condition that await just happens to "solve". I'd love a better solution if there is one.

@danielweck
Copy link
Member Author

danielweck commented Jan 3, 2022

it "worked", and seemingly continues to do so

Probably a side effect of Node's async_hooks, similar to why the Promise.resolve().then() wrapper seems to be necessary in prerender. Weird.

@rschristian
Copy link
Member

rschristian commented Jan 3, 2022

Here's a fresh reproduction repo, as the instructions I wrote above kinda suck: https://github.com/rschristian/twind-empty-duplicate-page-bug

Post install, this patches @twind/wmr's prerender so you don't have to. So install deps, run a build, and then notice that dist/bar/index.html is lacking all styles, when it should match 100% to dist/foo/index.html.

You can then add the await back into the module (node_modules/@twind/wmr/prerender/prerender.js) it it'll work, dist/bar/index.html has styles.

I haven't yet been able to break this, though I agree it's probably some sort of side effect that I'd think will fall apart eventually.

@danielweck
Copy link
Member Author

danielweck commented Jan 4, 2022

Thank you for the repro repo Ryan.

Regarding await sheet.reset() (or in my case, the lack of await which seems to make sense as reset() is not async): in the early days of my implementation I remember noticing inconsistent "bleeding" of styles (i.e. beyond the expected preflight CSS) from one prerendered page to the next, which didn't make sense at all, and more importantly which would by definition be an unreliable mechanism as visitors to my website wouldn't necessarily enter via the root index.html and/or in the order of generated Preact routes at build time. If I remember correctly, I hit this bug and quickly realised I had to implement my own logic anyway, due to the following:

Because of my specific functional requirements (notably: true zero-runtime Twind, i.e. Twind only used at build time), I decided early-on not to use use-twind-with/wmr. I am of course using the fundamental building block of Twind's stylesheet reset + repopulate at each WMR prerender pass.

My custom build system ensures that each prerendered SSR'd HTML file contains "critical" inline CSS sufficient for its own presentation, and that each such HTML file also references "secondary" external CSS for all other hydrated / CSR'd routes.

In order to completely remove Twind's runtime, I implemented additional pre- and post- WMR build steps that compute "critical" vs. "secondary" styles ... similar in spirit to Twind's own "extract CSS" utility, but not just based on static analysis of used classes, instead I use a Preact options.vnode -based logic that interprets dynamic Twind rules / possible component style variants depending on state, etc.

The "critical" inline styles are prerendered / SSR'd inside the head of any given static HTML file, and my build process also generates lower-priority / "secondary" external stylesheet links that provide CSS for the hydrated CSR'd pages of Preact WMR's isomorphic router (including Suspense dynamically-loaded Lazy components, I might add).

Crucially: there is no overlap / redundancy between "critical" and "secondary" stylesheet rules, in order to minimise footprint and network traffic. There is possibly a caveat with respect to the specificity of some CSS selectors, but so far I have not hit this edge case.

Anyway, long story short: I would have to look into the specifics of use-twind-with/wmr in order to figure out why await sheet.reset() seems to automagically fix the bug you describe, but my gut feeling is that this is an async_hook side effect.

UPDATE: I forgot to mention that ; of course ; the Twind "zero runtime" approach only applies to my WMR production builds, not the development server which does everything dynamically :)

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

No branches or pull requests

2 participants