Skip to content

Conversation

@rock-57blocks
Copy link
Contributor

@rock-57blocks rock-57blocks commented Dec 26, 2024

What this PR do?

As we talked here
There is a issue:
it just looks like the signal-effect update never gets flushed when there's a child depending on the signal, this might be because we first initiate a rerender in act which will queue up the useEffect and potentially override the scheduled one from the signal-effect.

Copy link
Member

@rschristian rschristian left a comment

Choose a reason for hiding this comment

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

Adding a test case would be appreciated if you can

@rock-57blocks
Copy link
Contributor Author

Adding a test case would be appreciated if you can

Sure, will do~!

@rock-57blocks rock-57blocks marked this pull request as draft December 26, 2024 04:37
@rock-57blocks
Copy link
Contributor Author

@rschristian
I've added some tests for the finish function but I also want to add tests with rendering component. Howerver, this issue only happened when using useSignalEffect, It seems that we should not introduce useSignalEffect in Preact tests
what do you suggest?

it.only("signals should not stop context from propagating", () => {
	let update: any;

	const MuteButton = ({
		isMuted,
			}) => {
		useEffect(() => {
				console.log('isMuted', isMuted);
		}, [isMuted]);

		return null;
			};

	const muteVideo = signal(false);
		const App = () => {
		const videoRef = useRef<HTMLVideoElement>(null);
		useSignalEffect(() => {
				console.log('running effect', muteVideo.value)
				videoRef.current!.muted = muteVideo.value;
		});
		console.log('rendering', muteVideo.value)

		return (
			<div>
				<video
				data-testid='video-player'
				ref={videoRef}>
				</video>
				<MuteButton
					isMuted={muteVideo.value}
				/>
			</div>
		);
			};

	act(()=>{
		render(<App />, scratch);
	})

	expect(scratch.innerHTML).to.equal('<div><video data-testid="video-player"></video></div>');
	expect(scratch.firstChild?.firstChild.muted).to.eql(false)
	act(() => {
		muteVideo.value = true;
	});
	expect(scratch.innerHTML).to.equal('<div><video data-testid="video-player"></video></div>');
	expect(scratch.firstChild?.firstChild.muted).to.eql(true)
});

Copy link
Member

@rschristian rschristian left a comment

Choose a reason for hiding this comment

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

Indeed, we shouldn't attempt to directly use useSignalsEffect in our tests here but we should be able to emulate it, (roughly) recreating the case in which act works incorrectly.

It's unfortunately pretty late here & I'm doing this phone, but your tests might cover this well enough already. I'll try to carve out some time in the next few days (holiday season, might be next week) to look into it more (if Jovi can't) as I'm not super familiar here.

Copy link
Member

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

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

The gist of what happens is

  • update signal
  • effect notifies (call options.raf)
  • rerender happens
  • useEffect deps change (call raf)

The tests sufficiently cover that

@rock-57blocks rock-57blocks marked this pull request as ready for review December 27, 2024 09:27
@coveralls
Copy link

Coverage Status

coverage: 99.616% (+0.001%) from 99.615%
when pulling a509f58 on rock-57blocks:fix-act-rerender
into 3618771 on preactjs:main.

@JoviDeCroock JoviDeCroock merged commit a32924c into preactjs:main Dec 27, 2024
13 checks passed
@JoviDeCroock JoviDeCroock mentioned this pull request Dec 27, 2024
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.

4 participants