Skip to content

Conversation

@welkinwong
Copy link
Contributor

This commit addresses an issue where component unmounting in React StrictMode was causing errors to be thrown. The problem was isolated to development environments only, as StrictMode additional checks are not performed in production. The fix ensures proper cleanup during unmount lifecycle without triggering these development-only warnings.

@nachocodoner nachocodoner requested a review from Grubba27 October 23, 2025 11:22
@Grubba27
Copy link
Contributor

Dang, you are on 🔥 !! very good job! I'll make a release with this change soon!


Somewhat related to these improvements, I know we somewhat handle the removal of items in the cacheMap, one thing I think we could explore (and if you think it makes sense) is to use WeakMap instead of Map? What do you think? I can make an issue explaining the reasoning

@Grubba27 Grubba27 merged commit 16b510e into meteor:feature/4.0.1 Oct 23, 2025
2 checks passed
@Grubba27
Copy link
Contributor

Published [email protected]

@Grubba27 Grubba27 mentioned this pull request Oct 23, 2025
@welkinwong
Copy link
Contributor Author

Somewhat related to these improvements, I know we somewhat handle the removal of items in the cacheMap, one thing I think we could explore (and if you think it makes sense) is to use WeakMap instead of Map? What do you think? I can make an issue explaining the reasoning

@Grubba27 I'm really sorry 🙏, I didn't notice this passage earlier.

Based on my current understanding, WeakMap is equivalent to a manually managed Map, with the major advantage of not having to worry about memory leaks, but the drawback is that primitive values can't be used and objects must be references.

One thing I can think of is that it could eliminate the key for suspense/useTracker (by holding the reactiveFn, but this requires that reactiveFn is wrapped with useCallback or similar). I had previously considered whether we could use reactiveFn.toString() as the key for the map.

We can continue discussing future improvements in the issue : )

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.

2 participants