defineProperty trap makes property non-CEW in React Native (Hermes engine)

[pastelmind]

Summary

This issue describes the bug reported in https://github.com/pmndrs/valtio/pull/752#issuecomment-1644968041 and #764. In both cases, Valtio was added to a React Native project with the Hermes engine enabled. Assigning a new value to a property on a proxy store object marked the property as non-configurable and non-writable, effectively "freezing" the property and preventing future mutations.

The source of the bug was initially attributed to #752. However, I suspect that the actual cause is #760. My own investigations (https://github.com/pmndrs/valtio/discussions/764#discussioncomment-6517634 and https://github.com/pmndrs/valtio/discussions/764#discussioncomment-6521744) indicate that the bug occurs on 1.11.0 (with #760), but not on 1.10.7 (with #752) or 1.10.6. Also, the bug affects proxy objects (affected by #760) rather than their snapshots (affected by #752).

The root cause appears to be a bug in the Proxy implementation of Hermes. I opened a bug report (https://github.com/facebook/hermes/issues/1065) and am waiting for confirmation from the Hermes devs.

Meanwhile, it would be nice if we handled this ourselves. Hermes and React Native are large projects and move slower than the rest of the JS ecosystem. Even if Hermes fixes the problem immediately (if at all), it would take a long time for the fix to propagate to downstream projects. Many people are using React Native today and we need to support them too.

Link to reproduction

See following discussion comments

• https://github.com/pmndrs/valtio/discussions/764#discussioncomment-6517634
• https://github.com/pmndrs/valtio/discussions/764#discussioncomment-6521744

Check List

Please do not ask questions in issues.

• [x] I've already opened a discussion before opening this issue, or already discussed in other media.

Please include a minimal reproduction.

• [ ] I've added a link to a typescript playground or codesandbox with a minimal reproduction.

[pastelmind]

I suggest two options:

1. Revert #760. Which would fix the issue but also make a useful pattern (#759) somewhat harder to use. I am OK with this since I can always fork and patch Valtio for personal use.
2. Alternatively, we can add a workaround for React Native.

But I'm hesitant for option 2 because the only workaround I can come up with is convoluted and brittle.

We already know that the `[[Set]]` internal method invokes `[[DefineOwnProperty]]` when the property already exists and is a data property. We could check this condition by detecting whether the `defineProperty` trap is invoked by the `set` trap: ```js // Inside proxyFunction() let isExecutingProxySetTrap = false const proxy = new Proxy(target, { set: (target, prop, value, receiver) => { isExecutingProxySetTrap = true try { // Actual set trap logic goes here return Reflect.set(target, prop, value, receiver) } finally { isExecutingProxySetTrap = false } }, defineProperty: (target, prop, desc) => { if (isExecutingProxySetTrap) { // If the defineProperty trap is triggered while processing the set trap, // then we know that 'desc' must have 'value' prop and nothing else // according to the ECMAScript spec. // So we can strip all other properties (erroneously added by Hermes) desc = { value: desc.value } } // Actual defineProperty trap logic goes here return Reflect.defineProperty(target, prop, desc) }, }) ``` Admittedly this is convoluted. It's also not foolproof; it might fail if the set trap invokes another set trap. In which case we'd need to use a counter instead of a boolean flag. But there may still be other pitfalls with this trick. Not to mention degrading performance.

For now, I suggest we wait for confirmation from another person that 1.11.0 is indeed the culprit. Then we can revert #760.

[lovetingyuan]

I make some tests and found something interesting.

import { Text, View } from 'react-native'
import React from 'react'
import { proxy, snapshot, useSnapshot } from 'valtio'

const store = proxy({
  foo: {
    1: 22,
  },
})

export default function App() {
  const { foo } = useSnapshot(store)
  const fooContent = JSON.stringify(foo, null, 2)
  return (
    <View style={{ flex: 1, marginVertical: 100 }}>
      <Text>foo: {fooContent}</Text>
    </View>
  )
}

If I change foo: { 1: 22 } to foo: { a: 22 }, there is no error. The difference is using number as key or not.

If I change useSnapshot to snapshot, there is no error, even using number as key.

[sharathprabhal]

This issue is still present in latest RN + latest valtio using the steps in https://github.com/pmndrs/valtio/issues/765#issuecomment-1647148884

@pastelmind how did you resolve the issue?

[pastelmind]

@sharathprabhal I do not use React Native, so unfortunately I cannot verify right now that the issue has been fixed. However, Valtio 1.11.1+ is supposed to be OK for RN.

Could you specify the exact version of Hermes/RN and Valtio, rather than "latest"? A reproduction repository would also help.

[sharathprabhal]

@pastelmind Thanks for the response!

The versions are

RN = 0.72.4
Valtio = 1.11.2

The repro repo is at https://github.com/sharathprabhal/valtio-765-repro/blob/main/App.tsx#L34 . Its a brand new RN app with the basic valtio store. Thanks in advance!

[ngvcanh]

I also had the same problem today:

import { proxy, useSnapshot } from "valtio";

export interface FilterState {
  brands: string[];
  gender: string[];
}

const state = proxy<FilterState>({
  brands: [],
  gender: []
});

export default function useFilter() {
  const snap = useSnapshot(state);

  return {
    ...snap,
    toggleBrand(brand: string) {
      if (state.brands.includes(brand)) {
        state.brands = [...state.brands.filter(b => b !== brand)];
      }
      else {
        state.brands = [...state.brands, brand];
      }
    }
  }
}

Error when I using toggleBrand function:

TypeError: ownKeys target is non-extensible but key is missing from trap result

RN: 0.72.5 valtio: 1.11.2

[dai-shi]

@pastelmind Are you around?

@sharathprabhal Maybe you want to open a new issue with the reproduction.

[pastelmind]

I'm not available at the moment. Hopefully I'll be able to dig into this before the weekend.

[sharathprabhal]

I was able to work around the issue by disabling Hermes. All set for now.

[lxfu1]

I also had the same problem today，Snapshot will cause loss of some abilities.