fix(vanilla): Prevent adding new property on snapshot

[pastelmind]

Related Issues or Discussions

Partially deals with #749 by disallowing property addition (but does not completely fix it, since we still allow property removal)

Summary

As a performance optimization, we currently mark properties of snapshots as configurable. This allows users to inadvertently add or remove properties on a snapshot object with no warning. Although TypeScript typings do prevent this to a degree, it is not foolproof.

While we currently do not have a robust solution that disallows property removal while maintaining the current level of performance, we can prevent addition of new properties on the snapshot by calling Object.preventExtensions() on it.

Check List

• [x] yarn run prettier for formatting code and docs

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
valtio	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 10, 2023 2:12pm

[codesandbox-ci[bot]]

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 135fdf0dc56892ada6c8701fea663a331a8b2a1c:

Sandbox	Source
React	Configuration
React Typescript	Configuration
React Browserify	Configuration
React Snowpack	Configuration
React Parcel	Configuration

[dai-shi]

Seems like #519 codesandbox works well too.

[dai-shi]

I'll make this to mark #749 as closed as nothing else is actionable. Please open a new discussion for more implementation ideas.

[lovetingyuan]

this change cause an error, call ownKeys in Proxy object, ownKeys target is non-extensible but key is missing from trap result

[pastelmind]

Could you elaborate? Please provide some sample code so we can understand it.

[lovetingyuan]

Could you elaborate? Please provide some sample code so we can understand it.

I use valtio in expo project, I think the problem may be related to [Hermes](https://reactnative.dev/docs/hermes) engine? I am not sure. Here is example repo: https://github.com/lovetingyuan/expo-example

setup development environment for expo, then run npm run android, click the detail button

[lovetingyuan]

when I add "jsEngine": "jsc", in app.json, there is no error. So this issue is highly possible related to hermes engine.

[lovetingyuan]

I create an issue in hermes repo https://github.com/facebook/hermes/issues/1063, you can take a look.

[pastelmind]

If this is an engine bug in Hermes, I'm OK with reverting the change for now while we investigate the issue.

[pastelmind]

From https://github.com/pmndrs/valtio/discussions/764 reported by @afkcodes, I hypothesize that Hermes may be implementing Object.preventExtensions() in such a way that it also makes existing properties non-writable and non-configurable, a la Object.freeze(). This could be wrong ofc. But if true, then we may have no choice but to remove Object.preventExtensions().

My investigations in https://github.com/pmndrs/valtio/discussions/764#discussioncomment-6517634 https://github.com/pmndrs/valtio/discussions/764#discussioncomment-6521744 indicate that the actual culprit may be a defineProperty proxy trap bug in Hermes. I suggest we continue this discussion in #765 as it is more visible than a closed pull request.

[afkcodes]

This i have validated and have found that this is true, i have my state typed, thanks @pastelmind