Issue with Input Chip reactivity

[endigo9740]

Current Behavior

InputChips stay invisible on clicking the corresponding option on the Autocomplete list.

Expected Behavior

InputChip should appear in list after selection by user.

Steps To Reproduce

1. Navigate to https://www.skeleton.dev/components/autocomplete
2. Scroll to section Input Chip
3. Click on one of the suggested options

Link to Reproduction / Stackblitz

No response

More Information

Discuss here, though the primary culprit is not immediately clear:

• bugfix: make inputchip values reactive #1918
• https://discord.com/channels/1003691521280856084/1144392201074393118/1144392201074393118
• https://discord.com/channels/1003691521280856084/1025279710609014805/1144615544914460766

[endigo9740]

This was the last notable change which may have introduced this regression:
bc9e3e8

[Hugos68]

Ok after some understanding of how this component works I figured why this is happening. The value variable is bound to the select element and keeps the internal state for what options are currently selected. chipValues is a variable that is supposed to reflect the value variable with and id (presumably used to key the each loop, ill get to that later). The chipValues is updated inside the addChip and removeChip functions, these functions are only called through submitting a chip/deleting a chip, when changing the value from the outside those functions are not called and therefor the internal state is out of sync with the chipValues.

Now that we/I understand the problem I propose the following fix:
Replace:

{#each chipValues as { id, val }, i (id)}

with

{#each value as val, i (val)}

and remove the chipValues entirely.

Explanation: I assume the chipvalues variable was added in order to add an id to each chipValue for the keyed each loop to work (animate:flip only works with a keyed each loop). Instead we can just use the val instead as a key. So far I've tested this and don't see any issues with it. Let me know what you think of this.

If this fix isn't suitable and a unique generated id is required we would need to implement a sync method of some sort that is called whenever value changes (since it can be changed from the outside. This would look something like this:

function sync() {
    // chipValues is synced with value here
}
$: value, sync();

This would call sync whenever value changes (which it does in both the addChip/removeChip functions as well as being changed from the outside therefor keeping the chipValues in sync).

Let me know your thoughts :)

PS: I have a branch in my fork having the changes from solution 1, if this suffices as a solution I will create a PR

[Hugos68]

Alright, solution one won't work due to the duplicate value option, will work on solution two for now, see If I can come up with something clever.

[ak4zh]

@Hugos68 Since you are working on this please do take a look at #1918 if you haven't already.
The bc9e3e8 removed the reactivity because it was being triggered twice.

The reason it was being triggered twice was as you noted above once by the function and another by reactivity.
Now as you noted above that values can be changed from outside of the function the correct way would be to keep reactive and remove updating chipValues from inside function so it does not trigger twice.

[Hugos68]

@Hugos68 Since you are working on this please do take a look at #1918 if you haven't already. The bc9e3e8 removed the reactivity because it was being triggered twice.

The reason it was being triggered twice was as you noted above once by the function and another by reactivity. Now as you noted above that values can be changed from outside of the function the correct way would be to keep reactive and remove updating chipValues from inside function so it does not trigger twice.

Currently working on that! Thanks for the heads up!

[Hugos68]

Ok I've been working on this for 4 hours now and discovered some things:

1. Making chipValues reactive based on value is not possible because we don't know what updated on value.
2. If duplicates were not a thing, this would perfectly possible without need of any external array like chipValues since we can use value as the key as it would be unique.

I would like to propose to remove the duplicate feature, I've been thinking about it and discussing it with some other programmers (I couldn't wrap my head around the issue) and it really doesn't make a lot of sense to require duplicates for a tag input, please let me know what you think about this. If this simply won't happen there is still a fix for this issue which I am working on right now.

[endigo9740]

@Hugos68 it's been a bit since I touched this feature. Can you explain what you mean by "duplicate feature"? Reviewing the docs and props I see nothing labeled this. Not sure I follow.

[Hugos68]

The fact that it allows for duplicate tags to be passed so I can have 2 tags that have the same value, if this weren't the case we wouldn't need to add artificial id's since we could you use the tag values themself, the reason we can't is because if the "allowDulplicates" prop.

[endigo9740]

We've had so much discussion around Autocomplete and InputChips this week that I've apparently just merged the two in my head heh. I was looking at the wrong component, my bad! That makes a million times more sense.

So the allowDuplicates feature is an odd one, but it's one that there is a use case for. Even if it's niche. For a contrived example, take the ice cream examples we use - someone could want three scoops of ice cream that's strawberry, strawberry, and then vanilla. If there's one example where this is needed, think of all the cases we're not thinking of.

Not to mention this would be a breaking change - so it would either have to go in before Monday (unfortunately won't happen) or would have to be reserved until v3. So the time table for that discussion is not great. Whatever solution we implement has to work with this feature in place.

I'll admit, I still haven't had a chance to sit down and wrap my head around the original issues trying to be addressed here. Could have something to do with it being nearly 3 am heh, but let's plan to just circle back on this next week after v2 ships. I'll have less on my plate and more badwidth to focus and provide real help here.

[Hugos68]

Alright, I see that use case being useful yeah, I would probably recommend to rollback Chad's change so that there is still a minor but known bug. This issue is a lot more complicated then one might think due to the nature of how svelte keyed loops work and how they are rendered

[djdembeck]

Do you happen to have any updates on this? I've downgraded to v1.11.0 as a workaround since Input Chips were broken entirely for us.

[endigo9740]

@djdembeck bare minimum, we're going to roll back the change that broke this for the v2 release this next week. But I'd like to take a deeper look when I can to try to resolve the the original issue that was meant to address as well.