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

Automatically detect if a block is using directives #126

Closed
luisherranz opened this issue Dec 23, 2022 · 12 comments
Closed

Automatically detect if a block is using directives #126

luisherranz opened this issue Dec 23, 2022 · 12 comments

Comments

@luisherranz
Copy link
Member

As mentioned by @SantosGuillamot here, we could Automatically detect if a block is using directives and add the wp-island functionality without forcing block developers to manually declare it in their block.json file.

@luisherranz luisherranz changed the title Automatically detect if a block is using directives and add the wp-island functionality Automatically detect if a block is using directives Jan 11, 2023
@luisherranz
Copy link
Member Author

Actually, there are two functionalities related to this:

  1. Check if a block has any directive to automatically mark it with wp-island for hydration.
  2. Check what directives are present in the HTML to enqueue their JavaScript files.

@ockham, @dmsnell: Let's discuss what mechanisms we can use for each of them 🙂

Maybe it's worth noting that both of them are optional, as we can always ask the developer to give that information to us. But the more we absorb, the better DX.

@ockham
Copy link
Collaborator

ockham commented Jan 11, 2023

  1. Check what directives are present in the HTML to enqueue their JavaScript files.

Based on my experiments, I'm expecting that we'll have a loop iterating over all HTML tags in a given block, which will be looking for wp- directives, and which will call the individual directive's processing code if it encounters one.

I was thinking to add logic to enqueue the relevant JS file(s) based on this, since it seemed like the most straight-forward way 😄

@dmsnell
Copy link
Member

dmsnell commented Jan 11, 2023

Given the cost of scanning HTML on every page render, it's likely that any solution will use more than one stage of detection: is it possible that this block contains directives? does this block contain directives?

So maybe a quick scan with a naive search, e.g. /[ <]wp-[a-z]/i, which tells us if there is anything in there that looks like it could be a directive, and then we start crawling the HTML looking for them.

Regarding attribute directives I'll repeat here that one thing we might consider is trying to add some known activator to indicate that a given tag has directives attached to it. Something like :wp: or wp-on in something like <div :wp: wp-show="…">

Far less a problem in client code where computation is spread out, the server is doing this processing for every page it hands out and any and all mechanisms to find directives is going to add its cost, but that's something to consider and acknowledge, not something that's a show-stopper.

@luisherranz
Copy link
Member Author

So maybe a quick scan with a naive search, e.g. /[ <]wp-[a-z]/i, which tells us if there is anything in there that looks like it could be a directive, and then we start crawling the HTML looking for them.

I was thinking about that, but block class names start with wp-block-... so we will mostly get false positives. I guess we'd have to remove the class from the HTML string first with something like preg_replace('/class=".*?"/g', '', $html).

Something like :wp: or wp-on in something like

At what part of the system do you envision those activators added? Are they delegated to the developer or added automatically at some point?

@dmsnell
Copy link
Member

dmsnell commented Jan 12, 2023

we will mostly get false positives. I guess we'd have to remove the class from the HTML string first with something like

This is a side-effect of using same prefix. I don't have estimates to quantify performance impacts so we just need to get testing with some realistic code and see what magnitude we're discussing. An attribute like :wp: circumvents this problem for the most part.

Also I didn't mean to be overly prescriptive in how a first-pass would be performed, only wanted to note that some kind of pass would likely be important given that we currently aren't talking about having any particular kind of thing we can use to narrow our search.

Unfortunately working to try and remove the class in some pre-processing would likely undo the advantage a first-stage pass would add.

At what part of the system do you envision those activators added? Are they delegated to the developer or added automatically at some point?

My first thought was that developers could add them as an opt-in (or if they use React components those directive components could add them). That's probably not a good idea because people wouldn't always do it.

We could try and add them automatically somehow, as a cache would work, or store them in the block JSON attributes on save. This could mess with folks in unexpected ways (broken editor, unexpected attributes, attribute name collisions), but it would leave these flags out of the HTML.

@luisherranz
Copy link
Member Author

Another option would be to use a different prefix to avoid collisions. Like wpx or wpi: <div wpx-show="…">.

This idea aligns better if we end up referring to this as a framework rather than just an API, an idea we are still tinkering with. The name of that prefix could match the framework's name (or its acronym).

@luisherranz
Copy link
Member Author

If we finally decide to propose data-wp- instead of wp-, it would also solve this problem.

Discussion: #132

@dmsnell
Copy link
Member

dmsnell commented Jan 23, 2023

Meant to come in and say I like the use of wpx, especially with the callback to htmlx, but then data-wp- sounds even better, potentially.

It wasn't clear to me that adding custom attributes is a violation, but after some searching I found the violation stated clearly with an example.

Authors must not use elements, attributes, or attribute values that are not permitted by this specification

with the purpose of this rule

doing so makes it significantly harder for the language to be extended in the future.

Still, it's one line in the spec and support is wide around the web for random attributes. I've noticed that while browsers tend to not allow setting arbitrary attribute names via setAttribute, they allow setting them via other HTML-parsing means like .outerHTML = , and happily return via getAttribute() the values of attributes it would refuse to set.

I don't think using something like wpx-show="…" would cause much harm, but data-wp-show="…" does seem bit better, and @luisherranz I doubt that the extra verbosity is going to be much of a problem. If people accidentally use wp-show instead it should fail consistently and immediately during dev so that should give someone an early alarm of the typo, just the same as if they typed wp-shoe by accident.

@dmsnell
Copy link
Member

dmsnell commented Jan 23, 2023

it seems to me that wpx might serve better than wp anywhere we're talking about this to differentiate it from other potential metadata we store that's WordPress related or derived, where wpx can indicate "the htmlx-like server-and-client language for dynamic content."

@luisherranz
Copy link
Member Author

I guess my mind wants to avoid any unnecessary boilerplate, even a five characters-long string like data- 😄

I doubt that the extra verbosity is going to be much of a problem

Yeah, I guess you're probably right. And convincing the WordPress community to go against the spec seems like a difficult task.

I've mentioned your opinion in the other issue as well. Thanks for your input, Dennis!

@dmsnell
Copy link
Member

dmsnell commented Jan 24, 2023

guess my mind wants to avoid any unnecessary boilerplate, even a five characters-long string like data-

It's okay. What your feeling is valid, but we must untrain our intuitions and learn to focus on the important things that matter 😄

convincing the WordPress community to go against the spec

I think the spec issue is probably overblown wording it this way. There's likely no system in the world that would break by adding custom attributes without the data- prefix. As @DAreRodz pointed out there are some intrinsic benefits to using data- attributes (and access via node.dataset wasn't even mentioned. while it's a small convenience, for attribute restructuring it can be useful).

Where I'd frame it is more like, given that both choices would work out, knowing that data- is closer to the spirit of how HTML wants to be extended, is there a strong reason to take the shortcut?

I think most libraries here violate the spec because prefer seeing hx-trigger="…" over data-hx-trigger="…". In the grand scale of things I doubt this matters too much for developers. Some will scoff at the extra five letters - no-prefix looks cleaner 🤷‍♂️.

So ultimately the choice is likely best made on other reasons than the spec; whereas the spec can be seen as a tie-breaker or a subtle nudge one way or the other.

@luisherranz
Copy link
Member Author

Now that the directives use the data-wp- prefix they should be easier to identify.

I'm going to close this conversation now. The task has been added to the new roadmap, and we'll open a new issue or discussion in Gutenberg once we need to start discussing its implications.

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

3 participants