Skip to content
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

feat: improve duplicate key error for keyed each blocks #8411

Merged
merged 4 commits into from Apr 18, 2023

Conversation

rafistrauss
Copy link
Contributor

@rafistrauss rafistrauss commented Mar 23, 2023

Closes #8410

Adds the duplicate key in the error message, to allow for easier debugging.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.

Tests

  • Run the tests with npm test and lint the project with npm run lint

Note
Lint passed, I ran npm test but saw the following 2 failures which I can't imagine were caused by my change:
image

@vercel
Copy link

vercel bot commented Mar 23, 2023

@rafistrauss is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

@Prinzhorn
Copy link
Contributor

Since keys can be anything this should probably use something similar to util.inspect to stringify the key into a human friendly version (maybe Svelte already has a primitive for this job, I'm not familiar). E.g. for Symbols your code will crash with:

Uncaught TypeError: Cannot convert a Symbol value to a string

@benmccann benmccann changed the title feat: emit the duplicate key as part of the error message when validating k… feat: improve duplicate key error for keyed each blocks Mar 23, 2023
@@ -112,7 +112,7 @@ export function validate_each_keys(ctx, list, get_context, get_key) {
for (let i = 0; i < list.length; i++) {
const key = get_key(get_context(ctx, list, i));
if (keys.has(key)) {
throw new Error('Cannot have duplicate keys in a keyed each');
throw new Error(`Cannot have duplicate keys in a keyed each: ${key} is a duplicate`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrap around with quotes to have a clearer idea of when the key starts and ends, especially when having string value as key.

Also, i would suggest add JSON.stringify() to differentiate between number and string value key

Suggested change
throw new Error(`Cannot have duplicate keys in a keyed each: ${key} is a duplicate`);
throw new Error(`Cannot have duplicate keys in a keyed each: \`${JSON.stringify(key)}\` is a duplicate`);

@gtm-nayan
Copy link
Contributor

For the problem that Prinzhorn mentioned, maybe using String() before inlining the key into the template string could be a workaround. For Symbols at least, I found that String(Symbol()) returns the string Symbol() instead of throwing that error.

@dummdidumm
Copy link
Member

Instead of trying to find a good stringified representation, I suggest to instead print out the index of the duplicates.

@dummdidumm dummdidumm changed the base branch from master to version-4 April 18, 2023 13:51
@rafistrauss
Copy link
Contributor Author

Instead of trying to find a good stringified representation, I suggest to instead print out the index of the duplicates.

Why not both? The string itself is much easier if your data is being reordered or transformed, so having that is the primary benefit in my mind. Then, if that approach fails due to the issues raised, provide the index as a fallback

@dummdidumm dummdidumm merged commit f30faa7 into sveltejs:version-4 Apr 18, 2023
11 of 12 checks passed
@gtm-nayan gtm-nayan mentioned this pull request Jun 17, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The dev error for duplicate keys in a keyed each should identify which key is a duplicate
5 participants