-
-
Notifications
You must be signed in to change notification settings - Fork 252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revert "feat(vanilla): Support class fields (defineProperty) (#760)" #766
Conversation
)" This reverts commit c0bda5a. Due to what I presume is a bug in the Hermes engine, React Native projects cannot use Valtio 1.11.0. Let's revert pmndrs#760 to support existing React Native users.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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 63b9cf6:
|
Is it now confirmed with #764 (reply in thread) ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good.
I believe so. |
Vote for this, when it could be merged? |
Is it worth reverting? Maybe the bug will be fixed in Hermes earlier. |
It makes sense to revert as we didn't know such a limitation when we merged and released.
So, there is an unexpected pitfall and it's harmful for some users. |
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
|
…mndrs#760)" (pmndrs#766)" This reverts commit 6eac2fc.
This reverts commit c0bda5a.
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 (c0bda5a). The following pattern (described in detail at #759) is no longer explicitly supported:
To work around this, we can explicitly set the field in the constructor, which properly invokes the
set
proxy trap: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
yarn run prettier
for formatting code and docs