Open electrostaticfleece opened 7 months ago
Thanks for the issue and discussion!
I don't really think there is a great solution here unfortunately. JS doesn't natively have protected
methods or properties. protected
would only actually help in Typescript applications, but wouldn't prevent folks from being able to call setValue()
at runtime. But, private
doesn't work well either, because if the method is JS native private
(like instance.#setValue()
then the implementing component that uses FormControlMixin
can't call it either.
I'm inclined to think that the best case is for consumers to just make sure not to add setValue()
to their documentation so that it won't show up as public even though technically it kinda has to be.
Hi, I would like to add to this request. In my project it would really help us if the properties that are meant to be protected, are also indicated as protected properties.
Our use case is that we generate a custom-elements.json
that generates our Storybook documentation and VS Code autocomplete. Now that these properties are not marked as protected, they get wrongfully added, which gives the impression that they are in fact public. That they are not truly private in compiled JS is less of an issue for us.
Thanks!
I can look into this but it is currently my belief that there is no way to indicate a mixin field/method as protected in TypeScript with proper intellisense support. If you have an example of that I’d be happy to take a look at this.
@calebdwilliams thanks for your quick response. When I mark a property as protected and try to use it from outside my class, I get the following error in VS Code:
Next to intellisense, we also generate a custom-elements.json
manifest. I have created a small example, where the card
class has two properties. The propertyMarkedProtected
is declared as protected, which adds "privacy": "protected"
to the custom-elements.json
manifest. This can be used by tools like Storybook to generate interface documentation, where members that are protected are ignored. You can find the example here: https://codesandbox.io/p/devbox/d6nxlf?file=%2Fcustom-elements-manifest.config.js
What I'm getting at is that you can't define a property or field as protected from the context of a mixin and have TypeScript recognize it.
That's why we provide a reference to the public API of the mixin as part of the generic argument to the mixin. Alternatively you can't provide protected metadata to an interface or I would have. It's different for base classes, but because we allow you to bring your own base class we don't have the same sort of flexibility that something like Lit does based off of TypeScript's design constraints.
I would love to solve for this, but I'm not sure TypeScript will allow us to, but if you can get a working example for a technique, I'd be happy to do a refactor.
@calebdwilliams ah I see what you mean. I did not know this was a limitation of the current TypeScript functionality. I researched a bit but I see there are still open issues for this on the TypeScript GitHub page. Thanks for clarifying!
Problem
The
FormControlMixin
currently exposes thesetValue
method as a public method on elements that use the mixin. When using theFormControlMixin
to build a published component for other teams to use, this can lead to confusion when consuming teams use TypeScript. SincesetValue
ispublic
it appears as though it is intended for direct use by said consuming teams.Impact
This misunderstanding may cause improper usage patterns and bypasses the intended abstractions we've designed for managing state and interactions within form control elements.
Proposed Solution
Changing
setValue
from apublic
method to aprotected
method would be ideal on our end. However, I understand this might not be ideal on your end. We're open to other potential solutions as well.