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.

Sign up for GitHub

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

feat: add readonly flag #92

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: add readonly flag #92

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions schema.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,11 @@ export interface Attribute {
* If the value is a string, it's the reason for the deprecation.
*/
deprecated?: boolean|string;

/**
* Whether the attribute is read-only.
*/
readonly?: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does it mean for an attribute to be read-only? afaiu, there really shouldn't be such a thing.

If there's a use for this, say to cover for the lack of CSS custom state, should we highlight that in the description?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an element might observe changes in that attribute and reject values it finds icky, resetting to the previous known-good value, or removing the attribute.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@justinfagnani Would this PR be clear to merge if it was only adding this to properties? I can create a new PR if necessary, but it would be nice to get this added for properties to unblock the CEM analyzer support. If @bennypowers or others feel strongly about usage with attributes, perhaps we can create a new issue for that discussion?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If youre using @custom-elements-manifest/analyzer, you can already support this via a plugin

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would go with this if it were for properties only to avoid semantics questions about "readonly" attributes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thepassle one tool emitting this property doesn't mean that other tools know to read it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but they mentioned cem analyzer being blocked, which it isnt

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to both of you for the responses. I am using that analyzer, and was able to find the plugin, so that does unblock me personally. My use case for this particular feature is Angular Framework integration, and I need to ensure that readonly properties can't be bound as an @Input().

I do still think it would be a good addition to the spec, so I'll try to get a PR up soon. Enough people have asked and it encodes a real semantic of the language. If it were added here, support could be added to the analyzer without a plugin, and other tools as well.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Id be okay with adding readonly for fields, im also unsure about attributes. Perhaps we can indeed address that in a separate discussion.

@MikeMatusz If and when that change has landed in the schema, would you be willing to contribute your plugin logic to cem/a?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not actually my logic, the original author of this PR appears to have written the plugin I found (https://github.com/bennypowers/cem-plugins/tree/main/plugins/cem-plugin-readonly), but I'd be more than willing to help contribute it to cem/a unless @bennypowers would prefer to do it himself. This spec and that analyzer have proven extremely useful to me.

}

export interface Event {
Expand Down Expand Up @@ -554,6 +559,11 @@ export interface PropertyLike {
* If the value is a string, it's the reason for the deprecation.
*/
deprecated?: boolean|string;

/**
* Whether the property is read-only.
*/
readonly?: boolean;
}

export interface ClassField extends PropertyLike {
Expand Down