Skip to content
This repository has been archived by the owner on Jul 28, 2023. It is now read-only.

Remove directive tags (components) from the proposal #152

Closed
luisherranz opened this issue Feb 17, 2023 · 11 comments
Closed

Remove directive tags (components) from the proposal #152

luisherranz opened this issue Feb 17, 2023 · 11 comments

Comments

@luisherranz
Copy link
Member

I'd like to propose removing the directives tags (also referred to as components) from the code and the proposal.

The reasons are:

  • During these months, we haven't been able to find a single use case, even a remote one, where using a directive tag is superior to using a directive attribute.
  • If we publish the directive tags as they are intended today, we may lose the opportunity to use them in the future for other uses.
@SantosGuillamot
Copy link
Contributor

Thanks for opening this discussion 🙂 It makes sense to me! If they don't add value, the fewer concepts we add, the better. If we remove directive tags, maybe we can just talk about directives, instead of directive attributes. This way, it should be easier to explain and understand.

@luisherranz
Copy link
Member Author

maybe we can just talk about directives, instead of directive attributes

I'd also love to hear opinions about this.

@DAreRodz
Copy link
Collaborator

I see value in using directive tags for components that are only rendered in the browser (e.g., a hypothetical <wp-graph type="column"> that renders a complex graph component and doesn't need server-side rendering).

Having said so, you can certainly use a "directive attribute" for that as well (e.g., <div wp-graph="column">) with the only consideration that you would probably have to pass custom props using data-* attributes.

maybe we can just talk about directives, instead of directive attributes

If we follow that path and later add the directive tags again, I think we'll not be able to name them "directives". 😅

@ockham
Copy link
Collaborator

ockham commented Feb 20, 2023

I'm all for removing 👍 For some directives, components/directive tags don't make sense at all (e.g. wp-bind and friends).

I see value in using directive tags for components that are only rendered in the browser (e.g., a hypothetical <wp-graph type="column"> that renders a complex graph component and doesn't need server-side rendering).

My 2 cents: YAGNI 😬 If it's only hypothetical, we don't necessarily need to accommodate for it yet; we can (re-)introduce the concept of a tag directive if/when we feel we actually need it.

maybe we can just talk about directives, instead of directive attributes

I'm all for that.

If we follow that path and later add the directive tags again, I think we'll not be able to name them "directives". 😅

I wouldn't rule that out! We could later introduce them saying that in addition to the previous directives, which were all attributes, we're adding directives that are tags/components themselves; to differentiate them, we'll refer to them as "tag directives", and to the "other" ones as "attribute directives". I think it's sometimes necessary to give an existing notion new meaning; that's fine, as long as we differentiate clearly.

@luisherranz
Copy link
Member Author

Thanks, folks.

Let's consider this decision taken and remove the directive tags/components from the code. I leave this issue open until it is done, but I'll move it to the pending tasks.

@dmsnell
Copy link
Member

dmsnell commented Mar 1, 2023

Hard to suggest not removing unnecessary complexity, but worth also noting that directive tags in general are faster to find than directive attributes. Practically speaking, the consequences here are probably something like needing to eventually special-case directives the Tag Processor in Core to support it.

For now it's all a wash, this is just an historical note.

@luisherranz
Copy link
Member Author

luisherranz commented Mar 2, 2023

Does it matter if we have to add directive attributes anyway?

EDIT: it's not a rhetorical question

@dmsnell
Copy link
Member

dmsnell commented Mar 2, 2023

No @luisherranz I guess it doesn't, and you're right. It's the presence of attribute directives that share a prefix with tag directives that prevents any speedups, but two prefixes for the same system for potential optimization seems unreasonable.

Long-term, if the goal is Core adoption, then special-casing is probably necessary anyway. The Tag Processor could expose $p->next_tag( array( 'contains_directive' => true ) ) and we could avoid the get_attribute_names_with_prefix() silliness that clones attribute names and returns an array only for us to check if it's empty.

@dmsnell
Copy link
Member

dmsnell commented Mar 3, 2023

@luisherranz I confirmed that Safari, Firefox, and Chrome all requested the images that were under a display: none div.

@luisherranz
Copy link
Member Author

I confirmed that Safari, Firefox, and Chrome all requested the images that were under a display: none div.

I confirmed that <template> doesn't, but I think this is the wrong issue, isn't it? 😄

@luisherranz
Copy link
Member Author

Solved in #217.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants