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

Separate $$ref from $$cache #30

Merged
merged 13 commits into from
Aug 6, 2023
Merged

Separate $$ref from $$cache #30

merged 13 commits into from
Aug 6, 2023

Conversation

SukkaW
Copy link
Contributor

@SukkaW SukkaW commented Jul 23, 2023

The PR does 2 things:

  • Revert back from useRef to useMemo for cache
  • Add a separate runtime $$ref to merging refs

This re-enables RSC support after 0.6.0.

@SukkaW SukkaW changed the title Separate ref and memo Separate $$ref from $$cache Jul 23, 2023
Copy link
Owner

@lxsmnsyc lxsmnsyc left a comment

Choose a reason for hiding this comment

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

Love that you made the effort. Added some reviews. Will consider this soon.

My only concerns would be:

  • Part of my decision on dropping useMemo is, as we know, how useMemo potentially resets state. Based on my observation, this doesn't seem to be the case for the production behavior, but more of the development behavior (related to react-refresh). So my concern still stands, as it affects the DX.
  • Minor thing is that this only solves RSC concern. But that's just "minor".

@@ -73,6 +73,7 @@ export default class Optimizer {
createMemo(
current: t.Expression,
dependencies?: t.Expression | (t.Expression | undefined)[] | boolean,
isUseRef = false,
Copy link
Owner

Choose a reason for hiding this comment

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

maybe instead of a boolean, you can just pass the union type? You had multiple ternaries after this.

Copy link
Contributor Author

@SukkaW SukkaW Jul 24, 2023

Choose a reason for hiding this comment

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

Fixed in ab18140 (#30).

packages/forgetti/test/__snapshots__/hooks.test.ts.snap Outdated Show resolved Hide resolved
@SukkaW
Copy link
Contributor Author

SukkaW commented Jul 24, 2023

  • Part of my decision on dropping useMemo is, as we know, how useMemo potentially resets state. Based on my observation, this doesn't seem to be the case for the production behavior, but more of the development behavior (related to react-refresh). So my concern still stands, as it affects the DX.

The new React Doc has explained when useMemo will throw away cache in detail: https://react.dev/reference/react/useMemo#caveats

And that's exactly why useMemo($$cache) and useRef($$ref) should be separated.

React expects useRef to hold the client-side only value (DOM Node, third-party libraries' instance like new VideoPlayer(), etc.), so refs will only be reset after components are unmounted.

React also expects useMemo for performance optimization, and React will throw away the cache when needed.

The PR makes the $$cache (and new $$ref) using the correct hook to meet React's expectations.

  • Minor thing is that this only solves RSC's concern. But that's just "minor".

Solving RSC compatibility is just a bonus as React's expectations have been met.

@SukkaW SukkaW requested a review from lxsmnsyc July 24, 2023 08:40
@SukkaW
Copy link
Contributor Author

SukkaW commented Aug 5, 2023

@lxsmnsyc Kindly request your attention with a friendly ping. I applied the fix to your requested changes 2 weeks ago. I would appreciate it if you could review it at your convenience and provide valuable feedback.

@lxsmnsyc
Copy link
Owner

lxsmnsyc commented Aug 5, 2023

@SukkaW Sorry I've been kinda busy recently. I'll give it a read tomorrow.

Copy link
Owner

@lxsmnsyc lxsmnsyc left a comment

Choose a reason for hiding this comment

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

Just a little bit of README adjustment is needed. All looks good to me.

packages/forgetti/runtime/index.ts Outdated Show resolved Hide resolved
packages/forgetti/src/core/presets.ts Show resolved Hide resolved
@SukkaW SukkaW requested a review from lxsmnsyc August 6, 2023 14:04
@lxsmnsyc
Copy link
Owner

lxsmnsyc commented Aug 6, 2023

Okay looks good. I'll release this some time this week.

@lxsmnsyc lxsmnsyc merged commit f5db6e9 into lxsmnsyc:main Aug 6, 2023
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