Skip to content

Conversation

rawagner
Copy link
Contributor

@rawagner rawagner commented Oct 9, 2025

…they did not change.

Previously getHookResults() always returned a new array, which may trigger unnecessary re-renders.
The hook now returns array of results directly, not a function (hookResults vs getHookResults), which aligns better with React hooks design (IOW return reactive state right away)

@rawagner rawagner requested a review from a team as a code owner October 9, 2025 13:15
@Hyperkid123
Copy link
Member

@rawagner The getHookResults() needs to be called manually the hook itself returns a memoized object, so that should never trigger any renders. Only when the render is forces. Have you encounterd issues with re-renders when using it?

@rawagner
Copy link
Contributor Author

rawagner commented Oct 10, 2025

That is exactly the problem I have - even though the manager itself is memoized, the getHookResults() return value is not.

Lets imagine this simple scenario:

const useMyHook = () => {
  const manager = useRemoteHookManager();
  
  useEffect(() => {
    manager.addHook({
      scope: 'foo',
      module: './bar',
    });
  }, [manager]);
  
  
  // The problem - the results are always a new array
  const results = manager.getHookResults()
  
  return useMemo(() => {
    //process results
  }, [results])
}

the useMyHook may rerender for whatever reason (not triggered by useRemoteHookManager), ie user simply clicks on a btn/async calls ends & sets a state somewhere. If you have the useMyHook in the rendering tree, it gets re-triggered - it calls manager.getHookResults() which always returns a new results.

@rawagner
Copy link
Contributor Author

rawagner commented Oct 10, 2025

Also if you compare the current memoization approach of useRemoteHookManager to, for example, React's useState - the useState does not memoize the returned array, it memoizes the value & setter refs. Whereas the useRemoteHookManager memoizes the object ( == useState's array) & functions, but does not give you a stable reference for the results.

The PR aligns the useRemoteHookManager to useState - returning object that is not memoized, gives you access to stable refs of results & functions.

@Hyperkid123
Copy link
Member

Hyperkid123 commented Oct 10, 2025

@rawagner OK understood. Can you re-install the dependencies please? I had a quick look already but I'll do a proper review later today. It did seem OK though :) Just wanted to know the details a bit more.

@rawagner
Copy link
Contributor Author

I've created a separate PR - there seems to be quite a few entries missing in the lockfile - unrelated to this change - unless Im doing something wrong :) #158

@Hyperkid123
Copy link
Member

Hyperkid123 commented Oct 10, 2025

@rawagner I think there might be issue with the relesae script, there was a bug with NX and lockfile updates (it was removing dev dependencies).

@Hyperkid123
Copy link
Member

@rawagner this PR should fix the lockfile issue on release: #159. Once merged, can you please rebase? (Already merged the updated lockfile PR)

Copy link

nx-cloud bot commented Oct 10, 2025

View your CI Pipeline Execution ↗ for commit 3822201

Command Status Duration Result
nx run test-app:serve ✅ Succeeded 1m 16s View ↗
nx affected -t component-test --configuration=ci ✅ Succeeded 8s View ↗
nx affected -t build ✅ Succeeded 22s View ↗
nx affected -t lint ✅ Succeeded 2s View ↗
nx affected -t test --configuration=ci ✅ Succeeded 3s View ↗

☁️ Nx Cloud last updated this comment at 2025-10-10 12:19:34 UTC

@rawagner
Copy link
Contributor Author

rebased, thanks @Hyperkid123

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