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

fix: memoize HMR, strict check constructors #2757

Merged
merged 5 commits into from
Feb 16, 2023
Merged

fix: memoize HMR, strict check constructors #2757

merged 5 commits into from
Feb 16, 2023

Conversation

drcmda
Copy link
Member

@drcmda drcmda commented Feb 14, 2023

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
Copy link

codesandbox-ci bot commented Feb 14, 2023

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
Copy link
Member

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
Copy link
Member

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

@CodyJasonBennett
Copy link
Member

CodyJasonBennett commented Feb 14, 2023

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
Copy link
Member

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?

@CodyJasonBennett

This comment was marked as outdated.

Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

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

Really great work both of you ⭐

@CodyJasonBennett
Copy link
Member

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
@CodyJasonBennett CodyJasonBennett changed the title fix: memoprop and hmr fix: memoize HMR, strict check constructors Feb 16, 2023
@drcmda drcmda merged commit 27d7164 into master Feb 16, 2023
@CodyJasonBennett CodyJasonBennett deleted the hmr branch February 16, 2023 22:46
renovate bot referenced this pull request in ziyadedher/ziyadedher Feb 17, 2023
[![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
[@&#8203;verekia](https://togithub.com/verekia) in
[https://github.com/pmndrs/react-three-fiber/pull/2761](https://togithub.com/pmndrs/react-three-fiber/pull/2761)
- fix: memoize HMR, strict check constructors by
[@&#8203;drcmda](https://togithub.com/drcmda) in
[https://github.com/pmndrs/react-three-fiber/pull/2757](https://togithub.com/pmndrs/react-three-fiber/pull/2757)
- fix: don't try to encode invalid sRGB textures by
[@&#8203;CodyJasonBennett](https://togithub.com/CodyJasonBennett) in
[https://github.com/pmndrs/react-three-fiber/pull/2762](https://togithub.com/pmndrs/react-three-fiber/pull/2762)

#### New Contributors

- [@&#8203;verekia](https://togithub.com/verekia) made their first
contribution in
[https://github.com/pmndrs/react-three-fiber/pull/2761](https://togithub.com/pmndrs/react-three-fiber/pull/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.

---

- [ ] <!-- rebase-check -->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).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xNDIuMSIsInVwZGF0ZWRJblZlciI6IjM0LjE0Mi4xIn0=-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
ziyadedher referenced this pull request in ziyadedher/ziyadedher Dec 16, 2023
[![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
[@&#8203;verekia](https://togithub.com/verekia) in
[https://github.com/pmndrs/react-three-fiber/pull/2761](https://togithub.com/pmndrs/react-three-fiber/pull/2761)
- fix: memoize HMR, strict check constructors by
[@&#8203;drcmda](https://togithub.com/drcmda) in
[https://github.com/pmndrs/react-three-fiber/pull/2757](https://togithub.com/pmndrs/react-three-fiber/pull/2757)
- fix: don't try to encode invalid sRGB textures by
[@&#8203;CodyJasonBennett](https://togithub.com/CodyJasonBennett) in
[https://github.com/pmndrs/react-three-fiber/pull/2762](https://togithub.com/pmndrs/react-three-fiber/pull/2762)

#### New Contributors

- [@&#8203;verekia](https://togithub.com/verekia) made their first
contribution in
[https://github.com/pmndrs/react-three-fiber/pull/2761](https://togithub.com/pmndrs/react-three-fiber/pull/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.

---

- [ ] <!-- rebase-check -->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).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xNDIuMSIsInVwZGF0ZWRJblZlciI6IjM0LjE0Mi4xIn0=-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
itsdouges pushed a commit to itsdouges/react-three-fiber that referenced this pull request Jan 2, 2024
* 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>
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.

3 participants