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

(preact-iso) - handle cancelled renders #527

Closed
wants to merge 5 commits into from

Conversation

JoviDeCroock
Copy link
Member

@JoviDeCroock JoviDeCroock commented Apr 3, 2021

By removing the synchronous render work we remove a few race conditions that were assumed to never occur, in this case when a pre-rendered route was used and we navigated away from it, the pre-rendered route would come back later and re-insert itself as the first child.

Fixes #425

CC @danielweck in case you have a mo, could you test this 😅 been doing a lot of testing but our tests don't seem to agree with me, mainly wondering if I'm missing something important

@changeset-bot
Copy link

changeset-bot bot commented Apr 3, 2021

⚠️ No Changeset found

Latest commit: a78ec2b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2021

Size Change: +170 B (0%)

Total Size: 734 kB

Filename Size Change
examples/demo/dist/about/index.html 669 B +2 B (0%)
examples/demo/dist/chunks/class-fields.********.js 202 B +2 B (+1%)
examples/demo/dist/chunks/compat.********.js 15.3 kB +1 B (0%)
examples/demo/dist/chunks/index.********.js 205 B +6 B (+3%)
examples/demo/dist/chunks/json.********.js 237 B +2 B (+1%)
examples/demo/dist/chunks/prerender.********.js 2.42 kB +2 B (0%)
examples/demo/dist/class-fields/index.html 648 B +2 B (0%)
examples/demo/dist/compat/index.html 1.5 kB +1 B (0%)
examples/demo/dist/env/index.html 725 B +3 B (0%)
examples/demo/dist/error/index.html 658 B +2 B (0%)
examples/demo/dist/files/index.html 689 B +1 B (0%)
examples/demo/dist/index.********.js 7.36 kB +127 B (+2%)
examples/demo/dist/index.html 716 B +2 B (0%)
examples/demo/dist/json/index.html 668 B +2 B (0%)
examples/demo/dist/lazy-and-late/index.html 671 B +3 B (0%)
packages/wmr/wmr.cjs 700 kB +12 B (0%)
ℹ️ View Unchanged
Filename Size Change
examples/demo/dist/assets/Calendar.********.css 702 B 0 B
examples/demo/dist/assets/style.********.css 386 B 0 B

compressed-size-action

@@ -3,23 +3,23 @@ import { useState, useRef } from 'preact/hooks';

export default function lazy(load) {
let p, c;
function inner(props) {
Copy link
Member Author

@JoviDeCroock JoviDeCroock Apr 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't mind the demo/public/lazy/*.js they weren't updated yet, best to look at the router for the actual changes

// It prevents the old route from being remounted because it got shifted in the children Array.
if (reverse && this.__v && this.__v.__k) this.__v.__k.reverse();
return () => {
if (cur.current.url !== url || pending.current !== p) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than synchronously checking whether the url has changed we need to do this within the effect, the url and mutative ref could be equal again in a scenario where we prerender --> lazy-load --> switch which would mean that we push the oldChildren back up

@danielweck
Copy link

CC @danielweck in case you have a mo, could you test this 😅 been doing a lot of testing but our tests don't seem to agree with me, mainly wondering if I'm missing something important

I tested this PR's branch.

I am afraid I can reproduce the bug consistently. The lazy-loaded component still appears below non-lazy routes, whenever I rapidly navigate from a lazy to a non-lazy route. Just to be clear: this is with production builds (npm run build --prerender && npm run serve) where a lazy prerendered route gets hydrated after its initial render.

Also note that if the hydration is allowed to complete (in other words: no clicking on one of the header links to interrupt the lazy route), the ErrorBoundary onError captures this exception: TypeError: e.__k is null. My observation has been (and still is) that navigating from the lazy route to a non-lazy component is safe ONCE this exception has been raised. Any navigation (or in my case, any useState() / "set state" hook interactions) before that point => bug. So my workaround consists in disabling the SPA router (i.e. ev.preventDefault() in my delegated listener which takes precedence over the router's own) until the user can safely click header links. This phase only takes a few milliseconds, at most a second, so the user experience does not degrade much.

I hope you will get to the root of this problem :) (you guys rock, by the way ... keep up the good work, please ;)

@developit
Copy link
Member

@JoviDeCroock I will push my take on this to a draft PR so we can compare. Needs tests.

@danielweck
Copy link

@JoviDeCroock that's an interesting / informative development, I think:
#530 (comment)

@JoviDeCroock JoviDeCroock deleted the handle-cancelled-renders branch April 10, 2021 09:44
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 this pull request may close these issues.

hydration of pre-rendered lazy route / component triggers exception, corrupts component tree
3 participants