hono icon indicating copy to clipboard operation
hono copied to clipboard

fix(jsx/dom): fix `memo` for DOM renderer

Open usualoma opened this issue 1 year ago • 2 comments

#3473 #3567

The jsx/dom memoization was generally behaving incorrectly, so I'm fixing it.

12ee8293ccb06dc3e0ae41116ebc412d925d3bac

In the past, the code used the same approach as for toString(), “holding memoized data in JavaScript memory,” but when rendering to the DOM, I found that memoization could be achieved by “not re-evaluate components” in the renderer, so the processing within the renderer was changed accordingly.

Also, looking at the code generated by React Compiler, it seemed that React expected the ReactNode object generated by jsx() to be cached and reused, and to be memoized, so I made it behave in the same way. I applied this branch to the code at https://github.com/sor4chi/explore-react-compiler-with-hono-jsx and confirmed that it was memoized.

38a3c7e5438c08b9036c3988e8932c2457bfb7b6 / 3b45b574d08890c8ead9bc8d8a093d4f6909b782

I changed the implementation of memo so that memoization is done using a new mechanism.

The author should do the following, if applicable

  • [x] Add tests
  • [x] Run tests
  • [x] bun run format:fix && bun run lint:fix to format the code

usualoma avatar Oct 27 '24 13:10 usualoma

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94.72%. Comparing base (7e11832) to head (ff69413). Report is 12 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3568   +/-   ##
=======================================
  Coverage   94.71%   94.72%           
=======================================
  Files         157      157           
  Lines        9539     9551   +12     
  Branches     2819     2830   +11     
=======================================
+ Hits         9035     9047   +12     
  Misses        504      504           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Oct 27 '24 13:10 codecov[bot]

Hi @usualoma

Looks good to me!

@sor4chi Are you interested in reviewing this PR? If so, please!

yusukebe avatar Oct 28 '24 06:10 yusukebe

I'll merge this now, though @sor4chi does not review it yet. Thanks!

yusukebe avatar Oct 30 '24 02:10 yusukebe

I was too busy to see it, sorry... Good test of react-compiler. I could confirm that it is working correctly for me too!

sor4chi avatar Oct 30 '24 13:10 sor4chi

@sor4chi Thanks for the review!

usualoma avatar Oct 30 '24 20:10 usualoma