Feature request: a check for closure+tsickle renaming safety

[rictic]

Short version: I'd like to implement a rule, disabled by default, that requires that properties declared with @property must be public, and that properties declared with the new @internalProperty must be private or protected. If the visibility does not match up with the decorator, I'd like to recommend that the code author use the decorator that matches up with their preferred visibility.

Longer version on the what and why: So at Google our TypeScript passes through tsickle to be come JavaScript, and then is compiled by Closure Compiler. Closure Compiler implements some really powerful optimizations like whole-program dead code elimination (not just tree shaking), but many of these optimizations make assumptions about the code. One of the most invasive such assumptions is property renaming, which will actually rename the fields on a class and everywhere that it's used. For custom elements, we don't want public fields to be renamed, because they could be used inside of raw HTML, in templates, etc, but it's ok if private or protected fields are renamed, because they shouldn't be used from HTML.

With LitElement, any field used with @property should not be renamed (after going through tsickle & closure compiler), but a field with @internalProperty may be. So we want to recommend the appropriate decorator for a given visibility. (We initially tried moving this complexity into tsickle, but the tsickle maintainers didn't want to make their tool any more complicated, so we've ended up with two decorators).

[rictic]

Any advice on implementing this? It looks like in the current architecture rules only apply to HTML, and most of the analysis of the declaration of a custom element takes place in webcomponent-analyzer. I could add the necessary information to webcomponent-analyzer and then add a check in lit-analyzer, but it would mean creating an additional kind of RuleModule.

Is this the architecture you'd recommend?

[runem]

I just finished adding support for @internalProperty in wca (this commit) :-)

In the past, it was the responsibility of wca to emit diagnostics for decorators/members, but I'm in the process of migrating these into lit-analyzer as rules. In this process, I'm adding a new kind of RuleModule that emits diagnostics for members. So I agree with the architecture that you suggest.

I'll let you know when I'm finished migrating the existing rules from wca to lit-analyzer using the new RuleType, - then I think it would be easy to implement the rule that checks visibility-modifiers + property/internalProperty :+1:

[rictic]

Awesome, great to hear!