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

10.18.2 regression: Cannot read properties of undefined #4194

Closed
1 task
zakstucke opened this issue Nov 6, 2023 · 8 comments · Fixed by #4281 or #4318
Closed
1 task

10.18.2 regression: Cannot read properties of undefined #4194

zakstucke opened this issue Nov 6, 2023 · 8 comments · Fixed by #4281 or #4318
Labels

Comments

@zakstucke
Copy link
Contributor

  • Check if updating to the latest Preact version resolves the issue

Describe the bug
Our dashboard starts fails when upgrading from 18.1 to 18.2. Looks like suspense boundary regressions.
I haven't got an easy repro or capacity to find one right now, just opening for visibility on the issue.

Screenshot 2023-11-06 at 13 23 22
@JoviDeCroock
Copy link
Member

@zakstucke any luck on the reproduction yet?

@zakstucke
Copy link
Contributor Author

@JoviDeCroock sad to say not yet! Like with the last suspense issue I helped with a while back, they seem to pop up in very specific circumstances and so all seems to work fine in a simple repo...

I've just checked with the latest version: 10.19.3 where issue still persists, last working version 10.18.1.

image

@zakstucke
Copy link
Contributor Author

Although looking at this new screenshot errs seem slightly different, now in signals, although this might be unrelated and from changes in my code.

@JoviDeCroock
Copy link
Member

JoviDeCroock commented Jan 11, 2024

@zakstucke I wonder if it's https://github.com/preactjs/preact/pull/4182/files we should check for the existence of oldVNode where does the stacktrace lead you in preact? or it has something to do with _nextDom setting with the insertBefore warning... not sure

@zakstucke
Copy link
Contributor Author

@JoviDeCroock @marvinhagemeister sorry to say that fix wasn't it!

But I've been useless for long enough, I've spent some time this morning trimming code until I got to a simple repro.

Repro made with npm preact init can be found here:
https://github.com/zakstucke/preact-4194-repro

This repro fails with 10.19.5, works with 10.18.1, and pretty sure it's the root of this issue.

cd my-preact-app && npm i && npm run dev

Going to http://localhost:5173 will give the following error:
image

The repro is effectively:

import { render } from 'preact';
import { Suspense } from "react";

import { Tab } from "@headlessui/react";

import { TestComp, TestWrapper } from "./lazyBarrier";

export function App() {
	return (
		<Suspense fallback={<p>Loading...</p>}>
			<Tab.Group>
				<Tab>
					<p>Foo</p>
				</Tab>
				<Tab.Panels>
					<Tab.Panel>
						<TestWrapper>
							<TestComp />
						</TestWrapper>
					</Tab.Panel>
				</Tab.Panels>
			</Tab.Group>
		</Suspense>
	);
}

render(<App />, document.getElementById('app'));

Where TestWrapper is a lazy loaded wrapper component (just returning children)
and TestComp is also lazy loaded and just returns <p>Bar</p>

I haven't been able to get @headlessui/react out of the mix, but it should be a start to diagnose!

Thanks

@JoviDeCroock
Copy link
Member

JoviDeCroock commented Feb 17, 2024

Hmm, I think that narrows it down to #4182, after some testing it does not seem to be the aforementioned issue. 4182 is a pretty big release so will look into this when I have some more time.

@JoviDeCroock
Copy link
Member

JoviDeCroock commented Mar 20, 2024

Okay, I had some time today to get to the bottom of this...

The main issue is that in nested fragment-like children we can have a very deep chain of components where the last one eventually resolves to unmounting the children.

This effectively means that _nextDom on all parents is going to be invalid, we however can't course-correct as we've already higher-up in the stack assigned oldDom https://github.com/preactjs/preact/blob/main/src/diff/children.js#L61 which now in-turn can become invalid by the first call to diff https://github.com/preactjs/preact/blob/main/src/diff/children.js#L85

@zakstucke
Copy link
Contributor Author

@JoviDeCroock I can confirm this actually fixed it! Thanks so much for all your help :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants