- Issue created by @fathershawn
- πΊπΈUnited States fathershawn New York
I had a conversation in Slack with @cilefen about how much we should present an abstract interface to the rest of the system and keep the HTMX implementation interface encapsulated. This alternate architecture would argue that we should name the facade class
Ajax
as that is the concept we are implementing. We would not provide direct access to theHtmxAttribute
andHtmxHeader
subsystems. The means of altering these subsystems would be to use an operation.Such an architecture only exposes HTMX concepts to developers that need to create a combination of attributes and headers that is not available as an operation. In that case, the developer would create a new operation to encapsulate that combination. Of course such a developer could forsake the facade altogether and deal with the attribute and header subsystem directly.
This idea could be take further in π Define and process an #htmx render array key Active and rather than define a new render array key, only add a new callback. The existing ajax callback would make an early return if the
#ajax
key did not point to an array and similarly the new callback if it did not point to an instance of the facade.What is the right architecture?
- π¬π§United Kingdom catch
iirc the original reason to have separate #ajax and #htmx render array keys was so that it would be easy to track converting from one to the other and eventually deprecating #ajax.
If we did something like this:
The existing ajax callback would make an early return if the #ajax key did not point to an array and similarly the new callback if it did not point to an instance of the facade.
Then presumably we could also issue deprecations for the old syntax that way. So as long as there's a way to clearly identify when which system we're using, then it comes down to what we want the eventual API to be like. I don't currently have a strong opinion between adding new things to #ajax before removing old things from #ajax, vs. adding #htmx in parallel and then eventually removing #ajax.
If there are ways to consolidate/reduce the list of AJAX commands that's great, the main thing is we need clear instructions for how to convert specific things from one to the other, even if it's no longer a 1-1 conversion.
- πΊπΈUnited States fathershawn New York
Thank you @catch. I wanted to layout the concerns expressed to me, and I'm comfortable with our original course. I think it is cleaner to create and transition to a parallel system. The
HtmxInterface
that I propose in the issue summary offers two paths for a developer.A developer does not need to know anything about how HTMX works. Choosing from a defined set of operations, such as Replace or Insert, the developer adds the required data to the operation and inserts it into the Htmx object.
Alternatively, a developer can dive completely into the world of HTMX using the attribute and header subsystem objects that are part of the Htmx object. I guess there is also a third path which is to create their own implementation of
HtmxOperationInterface
to package these into something reusable.This offers both simplicity and power in something that I think is straightforward to manage. Creating all the operations will be our final task in the initiative. I'm confident that we can offer a migration guide that maps ajax commands to htmx operations.
- πΊπΈUnited States fathershawn New York
Discussed and agreed in Slack that a single change record and release note will cover:
- π DX object to manage htmx attributes Active
- π DX object to manage htmx headers Active
- π DX object to collect and manage HTMX behaviors Active
- π Define and process an #htmx render array key Active
- πΊπΈUnited States fathershawn New York
Based on #12 and the description of the tag (A change record needs to be drafted before an issue is committed.) I'm removing the CR tag from this issue as these are sequential dependencies and the tag belongs on final component.
- @fathershawn opened merge request.
- πΊπΈUnited States fathershawn New York
All tests passing. Most of this MR is code moved from the the two prior RTBC issues. Comments on the MR indicate the overlap.
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
nod_ β credited larowlan β .
- πΊπΈUnited States phenaproxima Massachusetts
nod_ β credited phenaproxima β .
- π«π·France nod_ Lille
major since it's an important api addition β (for DX)
porting credits from the other issues too
- πΊπΈUnited States fathershawn New York
Updated the CR to match the revised approach.
- πΊπΈUnited States fathershawn New York
@nod_ Thanks for the ideas represented in your branch. I read through the changes and have some notes.
I removed the previously requested library optimization.
I profiled our two branches setting 10,000 string and 10,000 boolean attributes. In a single comparison they are within milliseconds of each other so there is no real performance difference.
The most important audience for the code, once it accomplishes its purpose, are humans. SinceAttribute
is written to accept values ofAttributeValueBase
I think explicitly creating attribute values of the correct type in an attribute factory clearly communicates what kind of attribute we are creating to the reader. That's valuable for immediate comprehension. If we need to go with your approach to get this committed, I'll support the pivot however I prefer the original code for these reasons.
Your change to the hx-name syntax from the data-hx-syntax reverses the decision in π± [policy, no patch] Choose a markup strategy for HTMX POC Fixed . I don't have a strong opinion either way but it surprised me. Should we re-open and document the change? - πΊπΈUnited States fathershawn New York
I was surprised at the performance similarity as the original approach will have less branching since we know the type to use. I then realized that the issue is in this code block in
Attribute
// If the value is already an AttributeValueBase object, // return a new instance of the same class, but with the new name. if ($value instanceof AttributeValueBase) { $class = get_class($value); return new $class($name, $value->value()); }
Recreating the object takes time. I don't find any instances of renaming created AttributeValue objects in core and the whole topic is out of scope for this issue, but moving the type check up like so:
/** * {@inheritdoc} */ public function offsetSet($name, $value): void { // If the value is not an AttributeValueBase object, then create one. if (!($value instanceof AttributeValueBase)) { $value = $this->createAttributeValue($name, $value); } $this->storage[$name] = $value; }
improved performance of the original code by 40%.
- πΊπΈUnited States fathershawn New York
fathershawn β changed the visibility of the branch 3524030-facade-simplified to hidden.
- πΊπΈUnited States fathershawn New York
Discussed, resolved questions and merged in remaining from #31 π DX object to collect and manage HTMX behaviors Active .
All tests passing.
- πΊπΈUnited States nicxvan
I updated the IS since it's not really an interface in drupal it's explicitly not an interface.
I confirmed everything lines up again.
I rescued the bits that were called out as different and they seem sensible.
I think this is ready except the one test that is final for some reason, I don't think that is worth holding this up over and expect it will be applied without controversy.
- π«π·France nod_ Lille
Just a sidenote about the attribute class
/** * {@inheritdoc} */ public function offsetSet($name, $value): void { // If the value is not an AttributeValueBase object, then create one. if (!($value instanceof AttributeValueBase)) { $value = $this->createAttributeValue($name, $value); } $this->storage[$name] = $value; }
A little problem with this is that
$name
could be different than what's$value
has been created with. Not sure why it'd happen but it's very easy to run into.In any case I'm happy with the MR so RTBC +1
- π¬π§United Kingdom catch
I didn't do a full review yet but have a couple of minor questions on the MR.
The Needs Review Queue Bot β tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide β to find step-by-step guides for working with issues.
- πΊπΈUnited States fathershawn New York
Responded to feedback from @catch. I left his MR threads open so that it is easy for someone else to re-check with a goal of returning to RTBC?