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

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

Open
bennypowers opened this issue Jul 21, 2020 · 6 comments · May be fixed by #126
Open

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

bennypowers opened this issue Jul 21, 2020 · 6 comments · May be fixed by #126

Comments

@bennypowers
Copy link

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
Copy link
Contributor

43081j commented Jul 25, 2020

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
Copy link
Owner

runem commented Jul 25, 2020

@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 👍

@pmcelhaney
Copy link

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
Copy link
Contributor

43081j commented Nov 2, 2021

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
Copy link

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

@h4de5
Copy link

h4de5 commented Dec 19, 2023

looking forward for a solution too.

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 a pull request may close this issue.

6 participants