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

feat: add readonly flag #92

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

Conversation

bennypowers
Copy link
Contributor

Closes #34

Copy link

@Westbrook Westbrook left a comment

Choose a reason for hiding this comment

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

@justinfagnani seems pretty straight forward? LGTM!

/**
* 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.

@justinfagnani
Copy link
Collaborator

Is this PR obsoleted by #118?

That one added readonly for PropertyLike, but not attributes, and #34 was closed by it.

I think if we can precisely describe what we want authors and tools to do with readonly attributes we could still do this.

@justinfagnani
Copy link
Collaborator

I'm still interested in discussing this, though it might be best in an issue. I'm mostly concerned with defining the semantics of "read only" attributes, since they technically don't exist in HTML. Under what conditions should tools add this flag? What behavior do we expect for tools that read it?

I wonder if it would be better named "output" or something similar - meaning that the element writes to the attribute (even though ideally they don't, but there are several reasons to at the moment, ie styling) so hosts of the element should not write to it.

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 this pull request may close these issues.

Add readonly flag to properties
5 participants