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

fix: Enable each to Handle Unions of Arrays #1218

Merged
merged 4 commits into from
Oct 28, 2021

Conversation

ITenthusiasm
Copy link
Contributor

@ITenthusiasm ITenthusiasm commented Oct 27, 2021

Example use case:

<script lang="ts">
interface OptionObject {
  label: string;
  value: string;
}

const options: string[] | OptionObject[] = [];
</script>

{#each options as option, i (typeof option === "string" ? option : option.value)}
  <div>{typeof option === "string" ? option : option.label}, {i}</div>
{/each}

Previously, the above code would cause a TS error within the each block, because the current version of svelte2tsx does not support unions of arrays. This PR resolves that problem.

Fixes #732.


I wasn't aware that the change was literally this simple. I would've submitted a PR earlier if I was aware. (Sorry 😬)

Notes:

  • I'd be happy to add new tests if desired. However, I would need some direction/hinting on where to add the tests, as there seem to be different groups of tests for different purposes.
  • yarn test still passes with this implementation. ✅ (I confirmed that yarn test fails if I make __sveltets_1_each an bad/inconsistent type.)
  • The instructions to get the extension running locally for me did not work after uninstalling the VS Code extension. However, the conversation on #732 proves that this solution works.

Happy to do additional work if needed.

Example use case:

```
const array: string[] | SomeInterface[] = [];
```
@dummdidumm
Copy link
Member

dummdidumm commented Oct 27, 2021

Thanks for the PR! Could you add a test that confirms the behavior by adding a test within DiagnosticsProvider.test.ts inside language-server? Basically you create a new diagnostics-each.svelte (or sth like that) inside the test files folder and test that it works. That file would test that it works for a simple type, works for a complex type like yours, and that it errors on non-array types.

From a quick test in the TS playground it seems that the value of an element in the array is any if the array is not ArrayLike. It was unknown before, throwing additional errors, but I would be okay with that since the other error would show that something is wrong anyway.

@ITenthusiasm
Copy link
Contributor Author

ITenthusiasm commented Oct 27, 2021

For sure! Just to be clear, is the test you described for manual testing? Or is it for automated testing?

For error cases, I'm not seeing yarn test fail, but I am seeing the expected types from IntelliSense.

Edit: Nevermind. I think I misunderstood. 😬

@ITenthusiasm
Copy link
Contributor Author

ITenthusiasm commented Oct 27, 2021

Okay, the tests should be added now. Let me know if anything more is needed.


Some notes:

  1. I can squash the commits (manually) if preferred (after everything is finalized).
  2. I can change the type to ArrayLike<unknown> if that's preferred. All the tests still pass. (I would just need to change the error messages from ArrayLike<any> to ArrayLike<unknown>. I tested this locally.) Or I can leave this as is. Whatever the preference is.

@ITenthusiasm
Copy link
Contributor Author

From the new commits, it seems like the preference would be unknown then. 👍

Let me know if anything else is needed on my part. Again, happy to squash manually to clean up the history when everything is done.

@dummdidumm dummdidumm merged commit f65d90e into sveltejs:master Oct 28, 2021
@dummdidumm
Copy link
Member

All good, I squash the commits when merging. Thank you for taking a look at this and fixing it!

@ITenthusiasm
Copy link
Contributor Author

For sure! Do you know when sveltejs/language-tools typically does releases? Just want to know where to keep track so I can update my codebase.

@ITenthusiasm ITenthusiasm deleted the fix/svelte-ts-each branch October 28, 2021 12:50
@dummdidumm
Copy link
Member

There's no regular schedule, it's more or less "has enough stuff accumulated and/or did enough time pass between two releases". I'll likely release tomorrow or early next week. When the referenced issue is closed, it's released.

dummdidumm pushed a commit to dummdidumm/language-tools that referenced this pull request Nov 16, 2021
Fixes a overzealous check introduced through sveltejs#1218
dummdidumm added a commit that referenced this pull request Nov 17, 2021
Fixes a overzealous check introduced through #1218
When checking JS, the check would infer the type of an array entry to unknown instead of any, when the type of the array was any
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.

Each Block Typing Issue
2 participants