fix(vanilla): respect property enumetrability

[alexander-entin]

Related Issues or Discussions

Fixes #726

Summary

Check List

• yarn run prettier for formatting code and docs

[vercel]

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	Oct 22, 2023 2:54am

[codesandbox-ci]

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 df29f66:

Sandbox	Source
React	Configuration
React TypeScript	Configuration
React Browserify	Configuration
React Snowpack	Configuration
React Parcel	Configuration

[dai-shi]

Please also check CI errors.

[dai-shi]

I think we are sure that it's not undefined.

Suggested change

	const { enumerable = true } =
	Reflect.getOwnPropertyDescriptor(target, key) ?? {}
	const desc = Object.getOwnPropertyDescriptor(
	target,
	key
	) as PropertyDescriptor

Do we want to make it true if it's undefined?

[alexander-entin]

1. As for getting property descriptor.
   We do not expect undefined and all we need is to make TypeScript happy.
   For this as PropertyDescriptor is indeed clearer than ?? {}

2. As for using the descriptor just got instead of creating a new one from scratch.
   No, this messes value, get, set and fails some tests.

3. As for enumerable default value.
   It looks like getOwnPropertyDescriptor always returns enumerable set.
   I don't know if this is guaranteed by the spec.
   At the same time defineProperty has its own defaults.
   I think we can assume that passing whatever is returned by getOwnPropertyDescriptor to defineProperty will do.
   So, we don't actually need to provide any default.
   Again, we only need to make TypeScript happy.
   I'm not familiar with TypeScript enough to propose a type-only solution.
   Maybe, using the same default as defineProperty is OK:

const { enumerable = false } = ...

[dai-shi]

If undefined is acceptable by TypeScript, let's go with it. (We just avoid using the knowledge what is the default.)

Suggested change

	const { enumerable = false } = Reflect.getOwnPropertyDescriptor(
	const { enumerable } = Reflect.getOwnPropertyDescriptor(

[dai-shi]

Maybe not? #800 (comment) seems to say so.

[alexander-entin]

const { enumerable } = Reflect.getOwnPropertyDescriptor(
        target,
        key
      ) as PropertyDescriptor
      const desc: PropertyDescriptor = {
        value,
        enumerable: enumerable as boolean,
        // This is intentional to avoid copying with proxy-compare.
        // It's still non-writable, so it avoids assigning a value.
        configurable: true,
      }

As I understand, this is correct from JavaScript view and doesn't rely on defineProperty defaults.
as boolean silences TypeScript (although, is not strictly correct type-wise)

[dai-shi]

LGTM. I think the TS hack is reasonable.