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

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

Closed
rschristian opened this issue Jan 3, 2022 · 7 comments · Fixed by #27
Closed

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

rschristian opened this issue Jan 3, 2022 · 7 comments · Fixed by #27

Comments

@rschristian
Copy link
Member

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

// Ensure to start a new async scope
return (data) =>
Promise.resolve().then(async () => {

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
Copy link
Contributor

sastan commented Jan 3, 2022

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
Copy link
Member

Somewhat related issue? #26

@danielweck
Copy link
Member

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
Copy link
Member

...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
Copy link
Member

danielweck commented Jan 3, 2022

UPDATE: NO!
See #25 (comment)

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 preactjs/wmr#893 (comment) )

@rschristian
Copy link
Member Author

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
Copy link
Member

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 #19 )

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.

3 participants