-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Rethink props #9826
Rethink props #9826
Conversation
🦋 Changeset detectedLatest commit: 9844243 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Beautiful
For what it's worth, I had previously tried a couple times to understand how |
Alright, this one was a real challenge. Hopefully people think it's worth it.
The goal here was threefold:
$$props
directly where possible #9813, we replaced function wrappers for the majority of props (those that aren't written to, and that don't have a default value). This PR takes that further — we no longer create a source for a prop unless a) it's written to and b) the prop wasn't declared withbind:
. This reduces memory cost, and the overhead associated withget
.sync_effect
dance. Using effects to keep these sorts of things in sync is best avoided —derived
is a better model. @trueadm was particularly keen to get rid ofsync_effect
(I'm not sure why it's worse than other types of effects, but I believe him)prop_source
was a bit of a beast (scary variable names likeignore_next2
!) and hopefully by separating out the three different scenarios (default only, bound, synced with local 'forking') it's a little bit less imposing now.Along the way, I uncovered a few interesting bugs:
sync_effect
dance. This allowed some tests to do things likelog.push(...)
inside an effect without triggering an infinite loop. In this PR, props and state behave more consistently, so I've rewritten those tests.sync_effect
, objects were being proxified eagerly. Without that, some tests failed. I've updated themHaving immersed myself in this for the last however many hours, I'm more convinced than ever that
accessors
needs to die. It makes everything more complicated than it needs to be, and reduces the usefulness of our test suite (by changing how props are compiled, thus reducing fidelity). I'll open an issue with a sketch of an alternative.Before submitting the PR, please make sure you do the following
feat:
,fix:
,chore:
, ordocs:
.Tests and linting
pnpm test
and lint the project withpnpm lint