QwikDev / qwik

Instant-loading web apps, without effort
https://qwik.dev
MIT License
20.7k stars 1.29k forks source link

[🐞] bind:value is broken when part of props spreading #3926

Open cmbartschat opened 1 year ago

cmbartschat commented 1 year ago

Which component is affected?

Qwik Runtime

Describe the bug

If I define:

const Input = component$((props: any) => {
  return <input {...props} />;
});

That doesn't work to set bind:value={xxx} on Input.

But this implementation does work:

const FixedInput = component$(({ "bind:value": bindValue, ...rest }: any) => {
  return <input {...rest} bind:value={bindValue} />;
});

Reproduction

https://qwik.builder.io/playground/#version=0.103.0&buildMode=development&entryStrategy=hook&files=JTVCJTdCJTIycGF0aCUyMiUzQSUyMiUyRmFwcC50c3glMjIlMkMlMjJjb2RlJTIyJTNBJTIyaW1wb3J0JTIwJTdCJTIwY29tcG9uZW50JTI0JTJDJTIwdXNlU2lnbmFsJTIwJTdEJTIwZnJvbSUyMCU1QyUyMiU0MGJ1aWxkZXIuaW8lMkZxd2lrJTVDJTIyJTNCJTVDbiU1Q25jb25zdCUyMElucHV0JTIwJTNEJTIwY29tcG9uZW50JTI0KChwcm9wcyUzQSUyMGFueSklMjAlM0QlM0UlMjAlN0IlNUNuJTIwJTIwcmV0dXJuJTIwJTNDaW5wdXQlMjAlN0IuLi5wcm9wcyU3RCUyMCUyRiUzRSUzQiU1Q24lN0QpJTNCJTVDbiU1Q25jb25zdCUyMEZpeGVkSW5wdXQlMjAlM0QlMjBjb21wb25lbnQlMjQoKCU3QiUyMCU1QyUyMmJpbmQlM0F2YWx1ZSU1QyUyMiUzQSUyMGJpbmRWYWx1ZSUyQyUyMC4uLnJlc3QlMjAlN0QlM0ElMjBhbnkpJTIwJTNEJTNFJTIwJTdCJTVDbiUyMCUyMHJldHVybiUyMCUzQ2lucHV0JTIwJTdCLi4ucmVzdCU3RCUyMGJpbmQlM0F2YWx1ZSUzRCU3QmJpbmRWYWx1ZSU3RCUyMCUyRiUzRSUzQiU1Q24lN0QpJTNCJTVDbiU1Q25jb25zdCUyMExpdGVJbnB1dCUyMCUzRCUyMChwcm9wcyUzQSUyMGFueSklMjAlM0QlM0UlMjAlN0IlNUNuJTIwJTIwcmV0dXJuJTIwJTNDaW5wdXQlMjAlN0IuLi5wcm9wcyU3RCUyMCUyRiUzRSUzQiU1Q24lN0QlM0IlNUNuJTVDbmNvbnN0JTIwVGVzdDElMjAlM0QlMjBjb21wb25lbnQlMjQoKCklMjAlM0QlM0UlMjAlN0IlNUNuJTIwJTIwY29uc3QlMjB2YWx1ZSUyMCUzRCUyMHVzZVNpZ25hbCglNUMlMjJ0ZXN0MSU1QyUyMiklM0IlNUNuJTIwJTIwcmV0dXJuJTIwKCU1Q24lMjAlMjAlMjAlMjAlM0MlM0UlNUNuJTIwJTIwJTIwJTIwJTIwJTIwJTNDaDIlM0VUZXN0JTIwMSUzQyUyRmgyJTNFJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUzQ0lucHV0JTIwYmluZCUzQXZhbHVlJTNEJTdCdmFsdWUlN0QlMjAlMkYlM0UlNUNuJTIwJTIwJTIwJTIwJTIwJTIwJTNDcCUzRSU3QnZhbHVlJTdEJTNDJTJGcCUzRSU1Q24lMjAlMjAlMjAlMjAlM0MlMkYlM0UlNUNuJTIwJTIwKSUzQiU1Q24lN0QpJTNCJTVDbiU1Q25jb25zdCUyMFRlc3QyJTIwJTNEJTIwY29tcG9uZW50JTI0KCgpJTIwJTNEJTNFJTIwJTdCJTVDbiUyMCUyMGNvbnN0JTIwdmFsdWUlMjAlM0QlMjB1c2VTaWduYWwoJTVDJTIydGVzdDIlNUMlMjIpJTNCJTVDbiUyMCUyMHJldHVybiUyMCglNUNuJTIwJTIwJTIwJTIwJTNDJTNFJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUzQ2gyJTNFVGVzdCUyMDIlM0MlMkZoMiUzRSU1Q24lMjAlMjAlMjAlMjAlMjAlMjAlM0NMaXRlSW5wdXQlMjBiaW5kJTNBdmFsdWUlM0QlN0J2YWx1ZSU3RCUyMCUyRiUzRSU1Q24lMjAlMjAlMjAlMjAlMjAlMjAlM0NwJTNFJTdCdmFsdWUlN0QlM0MlMkZwJTNFJTVDbiUyMCUyMCUyMCUyMCUzQyUyRiUzRSU1Q24lMjAlMjApJTNCJTVDbiU3RCklM0IlNUNuJTVDbmNvbnN0JTIwVGVzdDMlMjAlM0QlMjBjb21wb25lbnQlMjQoKCklMjAlM0QlM0UlMjAlN0IlNUNuJTIwJTIwY29uc3QlMjB2YWx1ZSUyMCUzRCUyMHVzZVNpZ25hbCglNUMlMjJ0ZXN0MyU1QyUyMiklM0IlNUNuJTIwJTIwcmV0dXJuJTIwKCU1Q24lMjAlMjAlMjAlMjAlM0MlM0UlNUNuJTIwJTIwJTIwJTIwJTIwJTIwJTNDaDIlM0VUZXN0JTIwMyUzQyUyRmgyJTNFJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUzQ2lucHV0JTIwYmluZCUzQXZhbHVlJTNEJTdCdmFsdWUlN0QlMjAlMkYlM0UlNUNuJTIwJTIwJTIwJTIwJTIwJTIwJTNDcCUzRSU3QnZhbHVlJTdEJTNDJTJGcCUzRSU1Q24lMjAlMjAlMjAlMjAlM0MlMkYlM0UlNUNuJTIwJTIwKSUzQiU1Q24lN0QpJTNCJTVDbiU1Q25jb25zdCUyMFRlc3Q0JTIwJTNEJTIwY29tcG9uZW50JTI0KCgpJTIwJTNEJTNFJTIwJTdCJTVDbiUyMCUyMGNvbnN0JTIwdmFsdWUlMjAlM0QlMjB1c2VTaWduYWwoJTVDJTIydGVzdDQlNUMlMjIpJTNCJTVDbiUyMCUyMHJldHVybiUyMCglNUNuJTIwJTIwJTIwJTIwJTNDJTNFJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUzQ2gyJTNFVGVzdCUyMDQlM0MlMkZoMiUzRSU1Q24lMjAlMjAlMjAlMjAlMjAlMjAlM0NGaXhlZElucHV0JTIwYmluZCUzQXZhbHVlJTNEJTdCdmFsdWUlN0QlMjAlMkYlM0UlNUNuJTIwJTIwJTIwJTIwJTIwJTIwJTNDcCUzRSU3QnZhbHVlJTdEJTNDJTJGcCUzRSU1Q24lMjAlMjAlMjAlMjAlM0MlMkYlM0UlNUNuJTIwJTIwKSUzQiU1Q24lN0QpJTNCJTVDbiU1Q25leHBvcnQlMjBkZWZhdWx0JTIwY29tcG9uZW50JTI0KCgpJTIwJTNEJTNFJTIwJTdCJTVDbiUyMCUyMHJldHVybiUyMCglNUNuJTIwJTIwJTIwJTIwJTNDJTNFJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUzQ1Rlc3QxJTIwJTJGJTNFJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUzQ1Rlc3QyJTIwJTJGJTNFJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUzQ1Rlc3QzJTIwJTJGJTNFJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUzQ1Rlc3Q0JTIwJTJGJTNFJTVDbiUyMCUyMCUyMCUyMCUzQyUyRiUzRSU1Q24lMjAlMjApJTNCJTVDbiU3RCklM0IlNUNuJTIyJTdEJTJDJTdCJTIycGF0aCUyMiUzQSUyMiUyRmVudHJ5LnNlcnZlci50c3glMjIlMkMlMjJjb2RlJTIyJTNBJTIyaW1wb3J0JTIwJTdCJTIwcmVuZGVyVG9TdHJpbmclMkMlMjB0eXBlJTIwUmVuZGVyT3B0aW9ucyUyMCU3RCUyMGZyb20lMjAnJTQwYnVpbGRlci5pbyUyRnF3aWslMkZzZXJ2ZXInJTNCJTVDbmltcG9ydCUyMCU3QiUyMFJvb3QlMjAlN0QlMjBmcm9tJTIwJy4lMkZyb290JyUzQiU1Q24lNUNuZXhwb3J0JTIwZGVmYXVsdCUyMGZ1bmN0aW9uJTIwKG9wdHMlM0ElMjBSZW5kZXJPcHRpb25zKSUyMCU3QiU1Q24lMjAlMjByZXR1cm4lMjByZW5kZXJUb1N0cmluZyglM0NSb290JTIwJTJGJTNFJTJDJTIwb3B0cyklM0IlNUNuJTdEJTVDbiUyMiU3RCUyQyU3QiUyMnBhdGglMjIlM0ElMjIlMkZyb290LnRzeCUyMiUyQyUyMmNvZGUlMjIlM0ElMjJpbXBvcnQlMjBBcHAlMjBmcm9tJTIwJy4lMkZhcHAnJTNCJTVDbiU1Q25leHBvcnQlMjBjb25zdCUyMFJvb3QlMjAlM0QlMjAoKSUyMCUzRCUzRSUyMCU3QiU1Q24lMjAlMjByZXR1cm4lMjAoJTVDbiUyMCUyMCUyMCUyMCUzQyUzRSU1Q24lMjAlMjAlMjAlMjAlMjAlMjAlM0NoZWFkJTNFJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUyMCUyMCUzQ3RpdGxlJTNFSGVsbG8lMjBRd2lrJTNDJTJGdGl0bGUlM0UlNUNuJTIwJTIwJTIwJTIwJTIwJTIwJTNDJTJGaGVhZCUzRSU1Q24lMjAlMjAlMjAlMjAlMjAlMjAlM0Nib2R5JTNFJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUyMCUyMCUzQ0FwcCUyMCUyRiUzRSU1Q24lMjAlMjAlMjAlMjAlMjAlMjAlM0MlMkZib2R5JTNFJTVDbiUyMCUyMCUyMCUyMCUzQyUyRiUzRSU1Q24lMjAlMjApJTNCJTVDbiU3RCUzQiU1Q24lMjIlN0QlNUQ%3D

Steps to reproduce

System Info

Qwik Playground/Windows/Firefox

Additional Information

No response

manucorporat commented 1 year ago

So, today bind:value is a compiler thing, it does not really exist as a prop, in general prop spreading is a practice that removes all compiler optimizations. I cant think of a way to make this work. But we should document it and print some warning in production.

Not sure this helps, but i cant think of an easy fix right now!

cmbartschat commented 1 year ago

If the compilation step is a simple bind:value => value/onInput$ translation, does that only apply to a specific set of elements like input/select? If that translation applied to the custom <Input> as well it seems like we'd be fine.

Another concern here is if I use styled components to define a StyledInput, will Qwik recognize that it needs the bind:value translation as well? Or is it only compatible if I set it on a raw <input>?

manucorporat commented 1 year ago

mmm, maybe for functional components we can leave the bind:value prop and add the value + onInput. ie, passing 3 props.

tzdesign commented 1 year ago

@cmbartschat looks like we have to do something like that:

 {bind && (
        <input
          {...rest}
          type={type}
          placeholder={placeholder ?? ''}
          bind:value={bind}
        />
      )}
      {bind === undefined && (
        <input {...rest} type={type} placeholder={placeholder ?? ''} />
      )}

Very ugly, but there is no other way.

@manucorporat This will be a crucial part for all UI component libraries. @shairez how do you guys do that with QwikUI ?

cmbartschat commented 1 year ago

Good workaround! I've just been doing this 🤦

export const Input = component$(
  ({ class: className, "bind:value": bindValue, ...rest }: Props) => {
    return (
      <input
        type="text"
        class={[serializeClass(className), styles.input]}
        bind:value={bindValue}
        {...rest}
      />
    );
  }
);

export const InputNoBind = component$(
  ({ class: className, ...rest }: Props) => {
    return (
      <input
        type="text"
        class={[serializeClass(className), styles.input]}
        {...rest}
      />
    );
  }
);
tzdesign commented 1 year ago

It's not really a good workaround. We will probably not use bind:value and omit it later. It's too confusing. The text field component is so complex (input or text area) that we will not duplicate the jsx for the sake of bind:value.

wmertens commented 8 months ago

Here's my workaround:

import {$, component$, type PropsOf} from '@builder.io/qwik'

export const TextField = component$(
    ({
        label,
        class: c,
        ['bind:value']: sig,
        ...rest
    }: {label: string} & PropsOf<'input'> & {type?: 'text'}) => {
        // workaround for spread props not working with plain tags
        const Input = 'input'
        return (
            <label class="form-control w-full">
                <div class="label">
                    <span class="label-text">{label}</span>
                </div>
                <Input
                    value={sig?.value}
                    onInput$={sig && $((__, el) => (sig.value = el.value))}
                    {...rest}
                    type="text"
                    class={['input w-full', !rest.disabled && 'input-bordered', c]}
                />
            </label>
        )
    }
)

@manucorporat why can't this be implemented in _jsxS?

wmertens commented 6 months ago

I took a look at this, the problem is adding a QRL from _jsxS, I can't make that work. If that works, the special handling can go out of the optimizer and into _jsxS, which makes it work even when passing via spread props.

maiieul commented 4 months ago

@wmertens I tried your workaround and it works for bind: but breaks the normal usage of value/onInput$

Here's my workaround:


type InputProps = PropsOf<'input'>;

export const Input = component$<InputProps>(
  ({ ['bind:value']: valueSig, value, onInput$, ...props }) => {
    return (
      <>
        <input
          {...props}
          value={valueSig ? valueSig.value : value}
          onInput$={valueSig ? $((__, el) => (valueSig.value = el.value)) : onInput$}
        />
      </>
    );
  },
);
JerryWu1234 commented 1 month ago

@maiieul What can I do here

maiieul commented 1 month ago

I have no idea :/ have you had a look at other tasks/issues?

wmertens commented 1 month ago

@maiieul What can I do here

I already have a fix halfway done but it requires qwik to be processed as a Qwik library and right now that's not happening. It just requires renaming the build to .qwik.js but it might cause issues.