mskocik / svelecte

Flexible autocomplete/select component written in Svelte,
https://svelecte.vercel.app
MIT License
447 stars 42 forks source link

Bug: Svelecte use the wrong labelField with valueAsObject #233

Closed jamesst20 closed 3 months ago

jamesst20 commented 3 months ago

Exemple

  const datas = [
    { id: 1, tag: "Tag 1", type: "Product" },
    { id: 2, tag: "Tag 2", type: "Product" },
    { id: 3, tag: "Tag 3", type: "Food" },
    { id: 4, tag: "Tag 4", type: "Food" },
  ];

  let values = $state([datas[0], datas[1]]);
<Svelecte
  bind:value={values}
  labelField={"label"}
  multiple={true}
  options={datas.map((data) => ({ ...data, label: `${data.type}: ${data.tag}` }))}
  valueAsObject={true}
  valueField={"id"}
/>

On render, every selected data will render with "undefined" text, however if I pick new items, they display just fine. It seems on render Svelecte pick up the text from the values instead of the option.

5.0.0-next.7

Proof

Using an existing field that I override in options:

<Svelecte
  bind:value={values}
  labelField={"type"}
  multiple={true}
  options={datas.map((data) => ({ ...data, type: `${data.type}: ${data.tag}` }))}
  valueAsObject={true}
  valueField={"id"}
/>

On page load the "type" is displayed, when I select an item it is properly formatted to ${data.type}: ${data.tag}

mskocik commented 3 months ago

Your example is confusing - you define values which don't have customized label/type property.

This is my (updated) version. But it's completely broken 🀯. Haven't tried it yet to run locally on the older svelte 5 version.

Which svelte 5 version are you on?

jamesst20 commented 3 months ago

Crap this is my mistake. I always forget that my wrapper around Svelecte magically do strictMode={!valueAsObject} to avoid this kind of confusion. I was confused myself for the past 30 minutes as I use everything in its latest version.

Very sorry for the wasted time.

By the way, it seems your $selected fix don't work so I added a demo aswell

Here is a proper REPL to replicate the issue: REPL

mskocik commented 3 months ago

to avoid this kind of confusion.

This is funny that even I got confused πŸ˜† - but I should know as component author, right?

Honestly I didn't "check" the correctness of the fix, I expect it will work. Will fix it properly this time. Thanks for bringing these issues to the light.

EDIT: sorry I edited your comment instead of quoting it - fixed πŸ˜†

mskocik commented 3 months ago

Okay... the $selected property issue is still here, but is this "labelField" issue valid? I can't reproduce it πŸ™„

jamesst20 commented 3 months ago

Okay... the $selected property issue is still here, but is this "labelField" issue valid? I can't reproduce it πŸ™„

You don't see "Undefined" ?

REPL

Displayed options should be taken from the options array and not the values itself no?

Capture d’écran, le 2024-06-04 aΜ€ 14 40 25

I would be expecting Svelecte to cross match my values with options to know what to display using the valueField

Edit:

In my use case, i'm creating Product Collections that are based on product tags. When I go to my edit page of Product Collections, /product_collections/:id/edit I receive by the backend the existing product collection and all the product tags.

Backend:

product_tags = [ { id: 1, tag: "Tag 1", type: "Product" }, ... ]
product_collection= {
  id: 1,
  product_tags: [
    { id: 1, tag: "Tag 1", type: "Product" },
    ...
  ]
}

I use Svelecte to update product_collection.product_tags however the label in the dropdown is not something that is "persisted" in the database, hence why i'm customizing the options. Also the options come from product_tags and not the product_collection itself as it is only containing a very few selected tags

mskocik commented 3 months ago

Displayed options should be taken from the options array and not the values itself no?

Yes, in general. BUT you are in valueAsObject mode AND providing initial value (strictMode={false}). Therefore it doesn't really use "options" version of given object, but the initial one.

Why is it done this way? you may ask and that's because it allows you to set initial selection object which is not present in options.


πŸ’‘ Your use case has a very simple solution - define your own render function, which will return desired label: REPL

jamesst20 commented 3 months ago

Displayed options should be taken from the options array and not the values itself no?

Yes, in general. BUT you are in valueAsObject mode AND providing initial value (strictMode={false}). Therefore it doesn't really use "options" version of given object, but the initial one.

Why is it done this way? you may ask and that's because it allows you to set initial selection object which is not present in options.

πŸ’‘ Your use case has a very simple solution - define your own render function, which will return desired label: REPL

Thanks for the explanation! I will use the renderer then :)

From my perspective, the only thing that should differ between valueAsObject true or false is that in one scenario it expect values to be option[valueField] and the other option itself. strictMode should always behave the same, thus checking for the presence of valueField in the options when enabled.

Feel free to close if you believe the current behavior is the expected one :)

mskocik commented 3 months ago

@jamesst20 Of course you are right. I got it messed up in my head with strictMode. I don't use objects as value very often. So to clarify: you don't need to use strictMode={false} in your use case.

Setting strictMode={false} is required only when following conditions (all at the same time) are met:

mskocik commented 3 months ago

Btw I still consider render function to be better approach than adding property label to options.

jamesst20 commented 3 months ago

@jamesst20 Of course you are right. I got it messed up in my head with strictMode. I don't use objects as value very often. So to clarify: you don't need to use strictMode={false} in your use case.

Setting strictMode={false} is required only when following conditions (all at the same time) are met:

  • use valueAsObject
  • use fetch
  • need to set initial value.

Thanks! version next.9 indeed solves properly the issue if strictMode is enabled! $selected is also gone.

Same REPL as before but now working

REPL

I agree that the renderer function is a better approach as I don't need to strip out my custom label from the payload prior sending to the server. This is what I am using but I am glad this could lead to some bug fixes.

Is it normal that it doesn't work with strictMode disabled? It's the expected behavior?

mskocik commented 3 months ago

Is it normal that it doesn't work with strictMode disabled? It's the expected behavior?

I am not sure I follow now. If you are refering to this previous comment πŸ‘‡ I fixed it as you described it should work. If you are talking about something else, please explain more, I am lost at the moment :)

From my perspective, the only thing that should differ between valueAsObject true or false is that in one scenario it expect values to be option[valueField] and the other option itself. strictMode should always behave the same, thus checking for the presence of valueField in the options when enabled.