useSyncEffect proposal Fix for #418

[Poyoman39]

This intends to fix #418
(Note that this is an early code proposal, juste to check if the idea is ok)

I think the "useSyncEffect" approach could solve the problem with useTracker leading to double rendering.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ Poyoman39
❌ Johan

---

Johan seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[nachocodoner]

Thank you for your contribution. Can you check if the tests pass after your changes? We may automate this as a GitHub action.

Test files are in that package, specifically for useFind:
useFind tests.

Also, consider expanding or modifying test coverage if needed for any new expectations introduced by these changes.

[Poyoman39]

Thank you for your feedback @nachocodoner. I tried and failed to run tests on my machine. Is there any documentation on how to run them ?

[Grubba27]

What failures did you have?

you ran using meteor test-packages ./path/to/package --driver-package meteortesting:mocha ?

[Poyoman39]

Thank you @Grubba27 for your help, i'm more a react dev than a meteor one for now.

I ran meteor test-packages ./packages/react-meteor-data --driver-package meteortesting:mocha locally, this output this error: Cannot find module "/node_modules/meteor/react-meteor-data/node_modules/react/index.js". Try installing the npm package or make sure it is not a devDependency

Event if my react-meteor-data npm modules are successfuly installed

[nachocodoner]

We are restoring test capabilities in this PR: #422

You can build on those changes, add them here, and run the tests to check for any impact.

I still need to review your solution further. Adding a timeout arbitrarily doesn’t seem like a good approach, I think.

[Poyoman39]

I still encounter this issue:

[[[[[ Tests ]]]]]

=> Started proxy.
=> Started MongoDB.
 (STDERR) packages/core-runtime.js:189
 (STDERR)             throw error;
 (STDERR)             ^
 (STDERR)
 (STDERR) Error: Cannot find module "/node_modules/meteor/react-meteor-data/node_modules/react/index.js". Try installing the npm package or make sure it is not a devDependency.

I'm using meteor 3 on my computer .. could this be the reason ?

[nachocodoner]

You merged it from master, the branch with tests fixed is tests-3.x. To run them, once you git checkout tests-3.x you should go cd tests/react-meteor-data-harness and run meteor npm test.

[Poyoman39]

Still running into this problem:

[PedroMarianoAlmeida]

I have another approach for this issue:
#423

[radekmie]

I checked the code now and I'm not sure I get the idea:

• Calling fetchData on mount in useEffect/useSyncEffect should not be needed, as the initial one is already covered in the initializer of useReducer.
• useSyncEffect is not really synchronous - it calls the effect function is useEffect's cleanup or in a setTimeout. And even if it'd be solved, I'm afraid it'd result in double render with (i.e., a flash of the old data and then the correct one).

[Poyoman39]

Hi @radekmie thank you for your review :)

• You are right, I guess with my solution we would not need anymore the first fetchData provided to useReducer
• UseMemo is run synchronously at component render time, so useSyncEffect is synchronous (only cleanup is asynchronous). Since const cleanup = useMemo(effect, deps); manage the effect, it won't be run a second time. Based on my tests i don't have a second render.

[Poyoman39]

The goal of useSyncEffect is to extract the logic of "i wan't to run the cursor.observe at render time but it's a bad practice and forbidden by react".
I think same problematic applies to useTracker and could be solved with a similar approach. (Currently useTracker cause a double render, and as i tested this useSyncEffect fixes this problem)

[radekmie]

Ah, right, I overlooked that it's a useMemo, sorry! And as for the second render, I lost track of how does triggering state change (here: dispatch) works in different React versions - maybe it's correct now.

[Poyoman39]

No problem, thank you for your review anyway :) I hope i finally find a way to run those meteor tests :D

[Poyoman39]

Thank you for your review @make-github-pseudonymous-again those are nice code change suggestions :)

Funny pseudo btw ^^

[make-github-pseudonymous-again]

Deduplicate cleanup calls, GC cleanup function reference, and handle undefined cleanup function:

Suggested change

	const cleanup = effect();
	let _cleanup = effect();
	const cleanup = () => {
	if (_cleanup === undefined) return;
	_cleanup();
	_cleanup = undefined;
	};

[nachocodoner]

What about adding more test coverage for this issue? In another PR, they provided a test that seems to catch this, showing it failing without the fix and passing when applied.

[StorytellerCZ]

I suggest merging #428, or at least merging that here and enabling the test.

[nachocodoner]

Could we get some effective test coverage for this? The test at #428 is already passing on master, even without the proposed changes here. An effective test should accurately capture the issue you’re fixing (#418). First, it should fail, showing the problem. Then, with the proposed changes, it should pass, confirming the fix. This process is the best way to verify and approve the fix. However, since this test already passes in base branch, we can’t confirm it’s effectively addressing the problem.

[PedroMarianoAlmeida]

The test is passing due to the early return on https://github.com/meteor/react-packages/blob/master/packages/react-meteor-data/useFind.tests.js#L113

This line should be removed on code change with the fix proposal

Note: I didn't want to add a failed test to master, so I did that just for who try to fix benefit for my test

[nachocodoner]

@PedroMarianoAlmeida: Oh, great aclaration. It is actually documented there, I misread it. Going to verify then. Thank you 🙏

[nachocodoner]

Tests are passing after make them fail and merge the proposed changes here in this PR: #433

Going to close this one in favor of the other. As I also solved the conflicts here as well. All your work and commits are perserved.

I'm going to approve it and merge it soon for the next beta release.

[nachocodoner]

This change is available in meteor add [email protected]. Please, verify it works as expected.