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

acceptHMRUpdate skips optional attributes without default values #2611

Open
bodograumann opened this issue Mar 13, 2024 · 3 comments
Open

acceptHMRUpdate skips optional attributes without default values #2611

bodograumann opened this issue Mar 13, 2024 · 3 comments
Labels
discussion HMR 🔥 Related to Hot Module Replacement

Comments

@bodograumann
Copy link
Contributor

bodograumann commented Mar 13, 2024

Reproduction

https://github.com/bodograumann/pinia-hmr-optional-attributes

Steps to reproduce the bug

  1. git clone https://github.com/bodograumann/pinia-hmr-optional-attributes
  2. cd pinia-hmr-optional-attributes
  3. npm install
  4. npm run dev
  5. xdg-open http://localhost:5173
  6. Press the "Increment" button. Sign says "positive".
  7. touch src/store/counter.ts

Expected behavior

All the state should be preserved.

Actual behavior

counter.nr is preserved as 1, but counter.sign is lost. The page shows -, while it should show positive.

Additional information

The relevant code is here:

if (!(key in newState)) {
continue
}

@posva
Copy link
Member

posva commented Mar 14, 2024

The problem is that the removal of the property sign could be intentional so I'm not sure if this is with changing as it would break other HMR cases.

In order to change this behavior, one could get each property with toRaw(newStore.$state) and check if the property is a ref in order to completely replace it rather than patchObject

@posva posva added discussion HMR 🔥 Related to Hot Module Replacement labels Mar 14, 2024
@bodograumann
Copy link
Contributor Author

Sounds to me like your proposed solution would keep the other use case of removing a property intact, right?
I think it should also work for us.
Normally I would be happy to contribute a PR, but given that my previous PR is still open and we are thus unable to use the latest pinia version, I am not sure how to proceed here 😐

Copy link
Member

posva commented Mar 15, 2024

I do need to check that PR. Currently we won’t be able to release a new version of pinia soon because of other changes 🥲

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion HMR 🔥 Related to Hot Module Replacement
Projects
None yet
Development

No branches or pull requests

2 participants