lit-plugin(no-complex-attribute-binding) when property has a converter

[bennypowers]

Hello!

My intended API is

// either
html`<trigger-mutation refetch-queries="OneQuery,AnotherQuery"></trigger-mutation>`
// or
html`<trigger-mutation .refetchQueries="${['OneQuery', 'AnotherQuery']}"></trigger-mutation>`

As such, I've defined refetchQueries as:

class TriggerMutation extends ApolloMutation {
  @property({ attribute: 'refetch-queries', converter: {
    fromAttribute(string: string): string[] {
      return !string ? undefined : string.split(',').map(x => x.trim()).filter(Boolean);
    },
    toAttribute(value: string[]): string {
      return Array.isArray(value) ? value.join(',') : null;
    },
  } })
  refetchQueries: string[];
}

Only to get this error:

You are assigning the primitive '"OneQuery,AnotherQuery"' to a non-primitive type 'string[]'. Use '.' binding instead?lit-plugin(no-complex-attribute-binding)(2)

As converter is intended to cast strings, perhaps this rule should be amended to allow ignoring properties with a converter or with type: Array or type: Object.

[43081j]

I can have a go at this, though question for @runem: do we keep any context around the original property definition? by the time our visitors get the assignment, the target will just be a typed property will it not? i.e. it won't have any knowledge by that point of if there's a converter or not.

my suggested fix is:

• change the no-complex-attribute-binding to check if there is a converter and, if there is, add string to the target type it checks against (so it would become string[]|string in this case)
• possibly change is-assignable-in-attribute-binding to do similar, but given this doesn't seem to have problems here maybe it already handles it fine

[runem]

@bennypowers Thanks for reporting this issue :-)

Here are my thoughts:

• We don't have access to LitElement-related metadata if consuming custom elements from a library, so we have to think into this that we might now know the correct metadata in all cases.
• @43081j I like what you have done in the PR for no-complex-attribute-binding by opting out of the check for Only primitive types should be allowed as "typeA" if the attribute has a custom converter.
• no-incompatible-type-binding will kick in with its own diagnostic if no-complex-attribute-binding is turned off/muted, so both rules should take custom converters account. I think we should just opt out of type checking if the attribute has a custom converter. This check could be introduced in the top of the isAssignableInAttributeBinding function. I don't think we need to type check assignability with typeA | string, because we know that typeB in an attribute binding will always end up being the type string and because we don't need to do custom type checking like in isAssignableToTypeWithStringCoercion.
• Regarding both rules, I'm a bit hesitant to always perform no (or loose) type checking for the two built-in converters type: Array and type: Object, because using the corresponding property makes it much easier to type check thus giving a better result with lit-analyzer. If we need to built support for this, I would very much like to see use cases for using Object and Array built-in converters with attributes instead of properties. For example, we might be able to introduce proper type checking if mylist="${JSON.stringify(myList)}" is the general pattern that is used/expected in this case.

@43081j Thanks for offering your help! I see that you already found out how to check for custom converters :+1:

[pmcelhaney]

I've run into the same problem. I'm curious why this code would ever be flagged.

html`<trigger-mutation refetch-queries="OneQuery,AnotherQuery"></trigger-mutation>`

The description is "Disallow attribute bindings with a complex type." I don't see any complex type there. I see an attribute bound to a string, which is the only type allowed in this context. An attribute value is always a string (a DOMString to be precise). We don't need to know anything about the implementation of the custom element to know that.

In contrast, this should probably be flagged by the rule:

html`<trigger-mutation refetch-queries="${['OneQuery', 'AnotherQuery']}"></trigger-mutation>`

because you most likely meant this instead:

html`<trigger-mutation .refetchQueries="${['OneQuery', 'AnotherQuery']}"></trigger-mutation>`

[43081j]

Been a while but iirc it means you have passed a string value (because you used an attribute binding) to a complex property (it is complex because in the class, it isn't a string).

Maybe it could be worded better

Totally forgot about this and the related PR I opened! I'll see if I can catch myself up on it again some time soon

[klasjersevi]

I just bumped into this problem. Any updates on this issue?

[h4de5]

looking forward for a solution too.