-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
fix: emit each_key_duplicate
error in production
#16724
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
Conversation
🦋 Changeset detectedLatest commit: ba50206 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 |
pnpm add https://pkg.pr.new/svelte@16724
I feel a bit weird about adding a less-cryptic error for when matched
is empty, but still not erroring at all if there are duplicate keys that don't result in an error. Is there some other proxy, besides matched
being empty, for those other cases?
I also realised as a result of this PR that validate_each_keys
doesn't always work, because the render_effect
could run after the block update — for the test case in this PR, it fails with a is undefined
. We could change that by turning the render_effect
in validate_each_keys
to a block
, but maybe it would be better to move the check into reconcile
inside an if (DEV)
block?
Checking other cases requires computations. We cannot even do array.length === state.items.size - to_destroy.length
(key duplication often results into a shortened list) because state.items
may still contain transiting items from previous runs. Probably the simplest computation is a set of seen keys in the reconcile
- validate keys during reconciling. (削除) Another place is inside If we want to add any computation to prod.create_item
. (削除ここまで)
Co-authored-by: Rich Harris <rich.harris@vercel.com>
Co-authored-by: Rich Harris <rich.harris@vercel.com>
I vote for merging this. I just ran into this and it's really annoying to debug. Yes it's not going to catch all cases but I'd rather fail with a runtime error that gives me a hint at what goes wrong compared to a cryptic "undefined" runtime error.
I tried to move validate_each_keys
inside reconcile
but some tests failed, and I didn't get time to dig into why.
I moved validate_each_keys
inside #each
's block
because, if thrown in reconcile
, the error isn't caught by the boundary in async mode. Guess it is because reconcile
is called in batch callbacks.
Btw, in prod the error isn't caught by boundary. I need help here.
Added the error emitting for the non-crashing case.
(削除) It doesn't emit the error at hydration in prod, but I guess it's fine. (削除ここまで)
Added ensuring the thrown error is caught by a boundary - is this the right approach?
I removed the tests because DEV is always true
and thus we cannot really test the prod checks.
No idea why the TSGo test decided to die on an unrelated file.
Uh oh!
There was an error while loading. Please reload this page.
Closes #15339
It looks like duplicate keys cause crashing only in one place.
In some cases it still can successfully render the list but will crash soon anyway, so I don't see reasons to add other checks.
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 test
and lint the project withpnpm lint