Revert "feat(vanilla): Support class fields (defineProperty) (#760)"

pastelmind commented 1 year ago

This reverts commit c0bda5afd2780fb929656a87bcc9e16bcf189fd8.

Due to what I presume is a bug in the Hermes engine, React Native projects cannot use Valtio 1.11.0. Let's revert #760 to support existing React Native users.

Related Issues or Discussions

Fixes #765 May fix #764 (but we need confirmation from actual RN users)

Summary

This reverts #760 (c0bda5afd2780fb929656a87bcc9e16bcf189fd8). The following pattern (described in detail at #759) is no longer explicitly supported:

class ValtioProxy {
  constructor() {
    return proxy(this)
  }
}

class MyStore extends ValtioProxy {
  field1 = 'asdf' // OK
  field2 = []     // This works in v1.11.0.
                  // But when this PR is merged,
                  // the array will no longer be reactive
}

const store = new MyStore()
store.field2.push(1) // Will re-render views in v1.11.0
                     // but not after this PR is merged

To work around this, we can explicitly set the field in the constructor, which properly invokes the set proxy trap:

class MyStore extends ValtioProxy {
  constructor() {
    super()
    this.field2 = [] // This will be reactive
  }
}

Alternatively, you can configure your transpiler to use 'set' semantics for class fields (e.g. useDefineForClassFields: false. (Personally I advise against this, because it deviates from the modern ECMAScript spec)

Check List

[x] yarn run prettier for formatting code and docs

vercel[bot] commented 1 year ago

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 24, 2023 4:35am

codesandbox-ci[bot] commented 1 year ago

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 63b9cf647657d366fb219bca5993f72e9d82b268:

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

dai-shi commented 12 months ago

May fix https://github.com/pmndrs/valtio/discussions/764 (but we need confirmation from actual RN users)

Is it now confirmed with https://github.com/pmndrs/valtio/discussions/764#discussioncomment-6531628 ?

pastelmind commented 12 months ago

May fix #764 (but we need confirmation from actual RN users)

Is it now confirmed with #764 (reply in thread) ?

I believe so.

ivankdev commented 12 months ago

Vote for this, when it could be merged?

td-tomasz commented 12 months ago

Is it worth reverting? Maybe the bug will be fixed in Hermes earlier.

dai-shi commented 12 months ago

It makes sense to revert as we didn't know such a limitation when we merged and released. I don't disagree that this is a reasonable addition, but my assumption was it didn't hurt at all.

https://github.com/pmndrs/valtio/pull/760#pullrequestreview-1538331597

Not sure if there are pitfalls, but this should be harmless for normal (plain objects) usage, except for the bundle size increase.

So, there is an unexpected pitfall and it's harmful for some users.

dominiczaq commented 12 months ago

yeah, and there are a lot of ppl stuck in certain RN versions. And there is no timeframe for fixing this in hermes and even if it's fixed then it would still require people to upgrade to latest version

pmndrs / valtio