fix: memoize HMR, strict check constructors #2757

drcmda · 2023-02-14T19:19:18Z

for reference #2755

breaks testing, it relied on memoprops being present but tracking the logic gets too wired up. @joshuaellis do you remember what that was for?

codesandbox-ci · 2023-02-14T19:19:47Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ff538dd:

Sandbox	Source
example	Configuration

joshuaellis · 2023-02-14T20:05:24Z

It's needed for a test renderer. It's also used in react-dom - so I don't think we should remove it...

I didn't know it was being used at much in the r3f reconciler?

joshuaellis · 2023-02-14T20:09:02Z

In a few lines, what's the issue we're trying to solve here? 🤔

CodyJasonBennett · 2023-02-14T20:29:00Z

LocalState['memoizedProps'] is unrelated to #2755, but it was only used for HMR in R3F before this PR. bce99ae refactors RTTR to read prop values from the underlying fiber which is more correct, but if this is technically a breaking change (see snapshot diff), we can keep LocalState['memoizedProps'] as is and relegate this to v9 which already mirrors the Fiber['memoizedProps'] behavior. Alternatively, the new getMemoizedProps helper of RTTR can be inlined as a lazy getter on LocalState['memoizedProps'].

joshuaellis · 2023-02-14T20:54:23Z

LocalState['memoizedProps'] is unrelated to #2755, but it was only used for HMR in R3F before this PR. bce99ae refactors RTTR to read prop values from the underlying fiber which is more correct, but if this is technically a breaking change (see snapshot diff), we can keep LocalState['memoizedProps'] as is and relegate this to v9 which already mirrors the Fiber['memoizedProps'] behavior. Alternatively, the new getMemoizedProps helper of RTTR can be inlined as a lazy getter on LocalState['memoizedProps'].

We could omit attach though right? 🤔 By adding it to this line

That would solve part of the issue of potential breaking changes?

packages/test-renderer/src/__tests__/__snapshots__/RTTR.core.test.tsx.snap

joshuaellis

Really great work both of you ⭐

CodyJasonBennett · 2023-02-14T21:40:10Z

WRT #2755 (comment), we may want to keep memoizedProps but only as a getter for the current getMemoizedProps in RTTR until v9.

To be picked into another PR

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [@react-three/fiber](https://togithub.com/pmndrs/react-three-fiber) | [`8.11.1` -> `8.11.2`](https://renovatebot.com/diffs/npm/@react-three%2ffiber/8.11.1/8.11.2) | [![age](https://badges.renovateapi.com/packages/npm/@react-three%2ffiber/8.11.2/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/npm/@react-three%2ffiber/8.11.2/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/npm/@react-three%2ffiber/8.11.2/compatibility-slim/8.11.1)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/npm/@react-three%2ffiber/8.11.2/confidence-slim/8.11.1)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>pmndrs/react-three-fiber</summary> ### [`v8.11.2`](https://togithub.com/pmndrs/react-three-fiber/releases/tag/v8.11.2) [Compare Source](https://togithub.com/pmndrs/react-three-fiber/compare/v8.11.1...v8.11.2) #### What's Changed - docs: fix broken titles on events.mdx by [@verekia](https://togithub.com/verekia) in [pmndrs/react-three-fiber#2761 - fix: memoize HMR, strict check constructors by [@drcmda](https://togithub.com/drcmda) in [pmndrs/react-three-fiber#2757 - fix: don't try to encode invalid sRGB textures by [@CodyJasonBennett](https://togithub.com/CodyJasonBennett) in [pmndrs/react-three-fiber#2762 #### New Contributors - [@verekia](https://togithub.com/verekia) made their first contribution in [pmndrs/react-three-fiber#2761 **Full Changelog**: pmndrs/react-three-fiber@v8.11.1...v8.11.2 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/ziyadedher/ziyadedher).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix: memoprop and hmr * refactor(RTTR): get memoized props from instance fiber * refactor(RTTR): exclude instance props from snapshot * fix(RTTR): prefer live props * refactor: remove memoizedProps changes To be picked into another PR --------- Co-authored-by: Cody Bennett <23324155+CodyJasonBennett@users.noreply.github.com>

fix: memoprop and hmr

151abf0

drcmda mentioned this pull request Feb 14, 2023

When setting a transform related prop to undefined r3f noops the update instead of resetting to default values #2755

Closed

refactor(RTTR): get memoized props from instance fiber

bce99ae

refactor(RTTR): exclude instance props from snapshot

f38ed1c

This comment was marked as outdated.

Sign in to view

joshuaellis reviewed Feb 14, 2023

View reviewed changes

packages/test-renderer/src/__tests__/__snapshots__/RTTR.core.test.tsx.snap Outdated Show resolved Hide resolved

fix(RTTR): prefer live props

4169d8d

joshuaellis approved these changes Feb 14, 2023

View reviewed changes

refactor: remove memoizedProps changes

ff538dd

To be picked into another PR

CodyJasonBennett changed the title ~~fix: memoprop and hmr~~ fix: memoize HMR, strict check constructors Feb 16, 2023

drcmda merged commit 27d7164 into master Feb 16, 2023

CodyJasonBennett deleted the hmr branch February 16, 2023 22:46

timmevandermeer mentioned this pull request May 5, 2023

test renderer state is not correct since 8.11.2 #2856

Closed

CodyJasonBennett mentioned this pull request Sep 26, 2023

fix(applyProps): loosen copy identity in dev #3025

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: memoize HMR, strict check constructors #2757

fix: memoize HMR, strict check constructors #2757

drcmda commented Feb 14, 2023 •

edited

codesandbox-ci bot commented Feb 14, 2023 •

edited

joshuaellis commented Feb 14, 2023

joshuaellis commented Feb 14, 2023

CodyJasonBennett commented Feb 14, 2023 •

edited

joshuaellis commented Feb 14, 2023

This comment was marked as outdated.

joshuaellis left a comment

CodyJasonBennett commented Feb 14, 2023

fix: memoize HMR, strict check constructors #2757

fix: memoize HMR, strict check constructors #2757

Conversation

drcmda commented Feb 14, 2023 • edited

codesandbox-ci bot commented Feb 14, 2023 • edited

joshuaellis commented Feb 14, 2023

joshuaellis commented Feb 14, 2023

CodyJasonBennett commented Feb 14, 2023 • edited

joshuaellis commented Feb 14, 2023

This comment was marked as outdated.

joshuaellis left a comment

Choose a reason for hiding this comment

CodyJasonBennett commented Feb 14, 2023

drcmda commented Feb 14, 2023 •

edited

codesandbox-ci bot commented Feb 14, 2023 •

edited

CodyJasonBennett commented Feb 14, 2023 •

edited