Custom property Accessors linting issues with ts-lint

[Christian24]

Description

The docs show how to use custom accessors. Since RC3 requestUpdatehas to be called inside of the set accessor. TS-Lint always fails here, because requestUpdate returns a Promise that is never waited for and according to microsoft/TypeScript#14982 cannot be waited for anyway in accessors.

It would also be great to have an example in the docs how to achieve something similar with the@property decorator. I have figured out that putting the decorator on the getter works fine (see child2.ts in the stackblitz), but is it intended to be that way? I kinda like the idea of doing something similar to attribute, where you can overwrite the name of the property yourself, so the decorator could be put on the private class variable (that likely also has the initial value).

Live Demo

child.ts shows the implementation with @property:
https://stackblitz.com/edit/lit-element-hello-world-85lc6q?file=child.ts

child2.ts shows the get properties implementation:
https://stackblitz.com/edit/lit-element-hello-world-85lc6q?file=child2.ts

Expected Results

No linting issue when using custom accessors. Documentation on how to implement custom accessors with @property decorator.

Actual Results

linting issue with custom accessors.

Versions

• lit-element: v2.0.0-rc3
• webcomponents: vX.X.X

[Christian24]

I had another look at the source code: I think it would be an acceptable solution to not return updateComplete at the end of requestUpdate as the return value is never used in the source code and updateComplete is available as a get accessor anyway. I made this adjustment in my own fork already, will submit a PR soon.

[sorvell]

The return value is there merely for convenience, and this could be removed. That said, it seems pretty unfortunate that TypeScript is complaining about this.

@justinfagnani Any thoughts?

[daKmoR]

I think this is quite convenient as well

await el.requestUpdate();
// would become
el.requestUpdate();
await el.updateComplete;

or a little bigger example:

el.requestUpdate().then(() => {
  const text = el.querySelector('div > p > span').innerText;
});

// would become

el.requestUpdate();
await el.updateComplete.then(() =>
  const text = this.querySelector('div > p > span').innerText;
});

if possible it would be nice to keep it 🤗

if it's really that big of a problem I think we probably could survive without it 😛

[justinfagnani]

@sorvell it isn't TypeScript complaining about this, but a specific TSLint rule.

@Christian24 I think that rule is too aggressive to be useful. It's not possible to tell that an "unused" Promise is an error. I'd either turn off the rule completely or disable it on the line that calls requestUpdate().

[justinfagnani]

And I don't think we can break the API at this point. We pretty much have to close this.

[Christian24]

@justinfagnani I do not necessarily agree. I do agree TSLint may be a bit too aggressive here, but the general idea is very valid, because it helps spot all the areas where a promise is returned although one might not expect it (thinking about body.json() for example). Addressing this whenever it happens in code is a bit tedious. This might be a bit of a longer response, so sorry in advance. This is usually how the story of a custom accessor goes:

1. I have a property with a decorator (all my properties are that as standard)
2. I notice I need a custom accessor, so I implement static get properties for a single property. (I could put the @propertydecorator directly on the custom get accessor, but I am not sure if this is intended that way, so I always implement static get properties. It would be nice to put the decorator directly on the private/protected class field and just specify the name of the property there.
3. I implement the custom accessors.
4. I google the tslint-ignore flag and put it in the code.

This only became an issue after custom accessor wrapping was removed, before this it was so much more convenient.

Although it would be the inferior solution, maybe implementing two versions like requestUpdate() and requestUpdateAsync or the other way round would be an acceptable solution?