pmndrs / valtio

🧙 Valtio makes proxy-state simple for React and Vanilla
https://valtio.dev
MIT License
8.84k stars 248 forks source link

`underive` does not delete the derived property unless there is a dependency defined #685

Closed dlindahl closed 10 months ago

dlindahl commented 1 year ago

Summary

Calling underive with delete: true does not delete the derived property value unless there is a dependency.

  1. Calling derive subscribes to the dependencies defined within the derive function.
  2. Calling underive with delete: true loops through all of the dependencies.
  3. After removing each dependency subscription, the key of the subscription is added to a Set to delete later.
  4. Once each subscription has been removed, the derived functions that matched a subscription key are deleted from the proxyObject.

The problem arises from a situation where the derived function never calls get (for whatever reason) which prevents a subscription from being created. The lack of a subscription prevents the property from being deleted which then prevents new calls to derive because the property is already defined.

Is this silly? Yes. Was this hard to debug. Also yes. 😄

Property deletion should probably be decoupled from the existence of a subscription, especially since I see some async subscriptions being handled in the derive code which could possibly introduce a situation where underive is called before the async value resolves which would result in a similar error.

Link to reproduction

derive({
  myProperty(get) {
    return 123; // NOTE: Not using `get` which does not create a dependency.
  }
}, {
  proxy: myProxy
})

underive(myProxy, { delete: true, keys: ['myProperty'])
// Throws Error: object property already defined

Pardon the lack of a codesandbox repro as the problem seemed pretty straight forward. I can make one if you really need one though.

Check List

Please do not ask questions in issues.

Please include a minimal reproduction.

dai-shi commented 1 year ago

Nice catch. Thanks for reporting.

Would you or anyone like to fix it? derive is already complicated, and hope to simplify the implementation.

dai-shi commented 11 months ago

Here's a new proposition, which might be controversial: #792

dai-shi commented 10 months ago

Transferred to: https://github.com/valtiojs/derive-valtio/issues/1