chore: simplify props #16270
chore: simplify props #16270
Conversation
🦋 Changeset detectedLatest commit: 0ef587a 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 |
|
| } | ||
|
|
||
| return value; | ||
| } | ||
|
|
||
| if (has_destroyed_component_ctx(current_value)) { | ||
| return current_value.v; | ||
| // TODO is this still necessary post-#16263? |
There was a problem hiding this comment.
Pretty sure yes because things not read in a teardown can still cause bugs as seen in #16072 - we might need to make props signals after all, gonna investigate that soon
| // prop is written to, but there's no binding, which means we | ||
| // create a derived that we can write to locally |
There was a problem hiding this comment.
Found it strange that the "in Svelte 4 ..." logic above in the getter was removed - did some digging and turns out that #11577 was added back when we did return the getter above in legacy mode too. Now that logic is obsolete, but we should add a comment here explaining why we need it in legacy mode, too
| // prop is written to, but there's no binding, which means we | |
| // create a derived that we can write to locally | |
| // Either prop is written to, but there's no binding, which means we | |
| // create a derived that we can write to locally. | |
| // Or we are in legacy mode where we always create a derived to replicate that | |
| // Svelte 4 did not trigger updates when a primitive value was updated to the same value. |
There was a problem hiding this comment.
Turns out we actually don't need the runes/legacy check when the prop isn't written to — 3933328
There was a problem hiding this comment.
We do need it, the test hides the bug because it's using accessors by default. PR with fix incoming
#16270 removed a condition which seemed to keep passing the corresponding test, but it actually introduced a regression since the PROPS_IS_UPDATED is always set when accessors should be created, which is the case by default in legacy mode tests. Setting accessors to false in the test reveals the regression, so this reverts that part of the refactoring
#16270 removed a condition which seemed to keep passing the corresponding test, but it actually introduced a regression since the PROPS_IS_UPDATED is always set when accessors should be created, which is the case by default in legacy mode tests. Setting accessors to false in the test reveals the regression, so this reverts that part of the refactoring
While looking into #16263 I realised that we can drastically simplify the
propimplementation now that deriveds are writable. Some of the code in red is pretty hard to understand, but deleting it causes no tests to fail. Just to be safe, I added a changeset so that this creates a new version that we can roll back from if it causes any issues.Before submitting the PR, please make sure you do the following
feat:,fix:,chore:, ordocs:.packages/svelte/src, add a changeset (npx changeset).Tests and linting
pnpm testand lint the project withpnpm lint