search

hhvm / xhp-lib

Class libraries for XHP. XHP is a Hack feature that augments the syntax of the language such that XML document fragments become valid Hack expressions.
http://facebook.github.io/xhp-lib/
MIT License
1.38k stars 160 forks source link

Call transferAttributes() from :x:element on elements that implement HasXHPHelpers #130

Closed fredemmott closed 9 years ago

fredemmott commented 9 years ago

Now that https://github.com/facebook/xhp-lib/blob/master/src/html/XHPHelpers.php is in XHP-Lib, we should probably actually call the functions defined in it.

With the exception of the only-render-once logic, https://github.com/hhvm/xhp-bootstrap/blob/master/lib/xhp-bootstrap/base.php is probably a good starting point.

fredemmott commented 9 years ago

I'm planning on implementing this very soon.

In particular, I want transferAttributes to be called automatically, so that if something calls getID() on an XHP element implementing XHPHelpers, it's guaranteed that that ID is actually set on the node.

Swahvay commented 9 years ago

We talked about making transfer attributes functionality baked into core XHP components when Kyle and I first created the UI library at FB. We eventually decided against it because we felt that the functionality wasn't always desired and it was a lot more difficult to turn it off than to turn it on, so to speak.

But as I think about it more now, and how the use of XHP has evolved since those early days, maybe this is actually what we want. It does seem slightly awkward that it looks for a random helper trait though. And I guess a weird thing to me is how it special-cases the HTML class attribute. This means that XHP is now focused solely on HTML rendering. It may actually cause unexpected results when used outside that context. Some other contexts are XML, RFC 5545, or even using it as AJAX responses. These are all valid use cases XHP can really simplify in PHP.

I guess maybe it would make more sense to me if we renamed XHPHelpers to something like XHPHtmlHelpers, that way it's a little more clear where these functions are really meant to be used. I dunno though, transfer attributes is a really handy thing to have on-hand.

Oh, also, there are times when we want to treat other attributes besides just class as special casing. In Javelin, they rely on the sigil attribute, which is space-separated string values just the same as class. They even have an addSigil method in XHP. How would we handle that case here?

fredemmott commented 9 years ago

How about we change the current XHPHelpers marker interface to extend a new XHPHasTransferAttributes interface? That way x:* stay unaware of 'class', and 'sigil' could also be added in another trait (or base class) without having to modify the core.

Swahvay commented 9 years ago

Yeah, I think that's a good solution.