tw-in-js / use-twind-with

Twind integration packages for frameworks & libraries with examples
MIT License
68 stars 17 forks source link

[WMR]: Stylesheets not clearing between pages being prerendered #25

Closed rschristian closed 2 years ago

rschristian commented 2 years ago

@sastan: do you happen to remember the context of this comment?

https://github.com/tw-in-js/use-twind-with/blob/d40071a0d91319d4a07ceb09b4a03e4f4d7c729d/packages/wmr/prerender.ts#L22-L24

Running into an issue at the moment where previous styles aren't cleared from the stylesheet, and for whatever reason, this seems to be the culprit.

I've made a reproduction: https://github.com/rschristian/twind-uncleared-sheet-bug

This is just the WMR Example brought up to date and configured to also prerender a 'Not Found' page.

Once you build (npm run build), if you look at the built file (dist/404/index.html) you'll see that Twind has generated a rule for .bg-purple-400, even though it doesn't exist on any content in the page. It does, however, exist in /.

I'm not really sure what you're looking to do with that Promise.resolve().then(...). Replacing that and returning a plain async function seems to fix the issue.

// Ensure to start a new async scope
-return (data) =>
-    Promise.resolve().then(async () => {
+return async (data) => {
        await sheet.reset()
        let { html, ...rest } = await prerender(render(data), options)
        return { ...rest, html: getStyleTag(sheet) + html }
-})
+}
}
sastan commented 2 years ago

If it works great! I remember we had some problems with tracking of the current async scopes.

If it works with an async function now, we should change it.

danielweck commented 2 years ago

Somewhat related issue? https://github.com/tw-in-js/use-twind-with/issues/26

danielweck commented 2 years ago

Somewhat related issue? #26

Yep, this seems to be the culprit (I use my own use-twind-with implementation but at its core I use the same logic, so I was able to test the await bugfix).

danielweck commented 2 years ago

...also note that if a unit-test was to be written for this, Twind's config.preflight would need to be set to false (or maybe undefined or null ... I haven't tried all possible "falsy" values) in order to assert sheet.target.length === 0 (well, I use (Array.isArray(sheet) ? sheet : sheet.target).length but I can't remember exactly why! :)

danielweck commented 2 years ago

UPDATE: NO! See https://github.com/tw-in-js/use-twind-with/issues/25#issuecomment-1004422865

To Ryan's original point: -I removed- the Promise.resolve().then(async () => {}) wrapper in my prerender code, and everything seems to works fine (well, apart from the Node v16 random segfault ... but that's a completely separate problem https://github.com/preactjs/wmr/issues/893#issuecomment-1004408408 )

rschristian commented 2 years ago

Somewhat related issue? #26

Yep, this seems to be the culprit (I use my own use-twind-with implementation but at its core I use the same logic, so I was able to test the await bugfix).

I think you skipped looking over the blame that resulted in that line. Without it, pages that contain shared styles would end up missing it. Say /foo and /bar had text-white: Without #19, /bar would be missing that rule.

danielweck commented 2 years ago

To Ryan's original point: I removed the Promise.resolve().then(async () => {}) wrapper in my prerender code, and everything seems to works fine

OH! I retract this comment, sorry. This totally yields empty stylesheet in my tests. I am keeping the Promise.resolve() wrapper as this has been working great in my setup which is very similar to use-twind-with/wmr (I am also preserving the removal of await for sheet.reset() which has always worked fine at my end, I never experienced this issue https://github.com/tw-in-js/use-twind-with/pull/19 )