-
Notifications
You must be signed in to change notification settings - Fork 159
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
rewrite XHP guide for namespace support + xhp-lib v4 #901
Conversation
It seems I'll have to redo the review, since the comments are not bound to the line of code I made the comment for. |
guides/hack/21-XHP/04-setup.md
Outdated
## Namespace Support | ||
|
||
Support for namespaced XHP classes (elements) has only recently been added to | ||
HHVM (as of HHVM 4.68). Depending on the exact HHVM version, it may not be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version number is incorrect. HHVM has supported namespaced XHP components since at least hhvm 4.46, since xhp-lib's travis.yml tests on that version with namespace XHP components. Support may have been added earlier, but I am sure that 4.46 supports it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we should strongly discourage use of v4 with whatever version of HHVM have the current defaults, and strongly discourage v3 after the default is flipped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh I intentionally made it vague ("recently as of HHVM 4.68" -- though I guess ~4 months might be stretching the definition of "recently" a bit) because I wasn't sure what the exact version was, and it's probably impossible to pinpoint an exact version anyway (IIRC there have been some bugfixes and improvements after the support was first added...) but I agree, it would be better to clarify this.
That said, I also agree that we should discourage people from using XHP namespaces with HHVM versions that technically support them, but are not thoroughly tested (e.g. not used in production by any project), so I'd highly discourage using any HHVM version from before we converted at least the user-documentation site, and probably (like Fred said), from before these flags are on by default.
So probably something like: "...has been added around HHVM 4.46, but hasn't been enabled by default until HHVM 4.xx, so we recommend using HHVM 4.xx [same xx] or newer. In older HHVM versions, modern XHP features including namespace support can be enabled by adding the following flags to your .hhconfig
:" (also clarifies that some flags are technically unrelated to namespaces -- but given that we support/test either with none of them (xhp-lib v3) or with all of them (xhp-lib v4), I'd discourage people from using other subsets -- I also wrote that in the guide later)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also going to chagne xhp-lib's minimum HHVM version once the default flips, before 4.0.0 final - this is so that when people do composer require facebook/xhp-lib
, they get a version that works with the default settings. This isn't currently an issue as composer will not install an rc version unless you explicitly request it.
guides/hack/21-XHP/04-setup.md
Outdated
enable_xhp_class_modifier=true | ||
disable_xhp_element_mangling=true | ||
disable_xhp_children_declarations=true | ||
check_xhp_attribute=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check_xhp_attribute
is not related to xhp-4. This was added in hhvm 4.8.0 and makes the following code a typechecker error, where this would previously be a runtime error during render.
final class :xhpclass extends :x:primitive {
attribute string href @required
}
<xhpclass />;
// Missing required attribute `href` on `xhpclass`.
guides/hack/21-XHP/04-setup.md
Outdated
|
||
``` | ||
$ composer require facebook/xhp-lib | ||
# note: make sure to use PHP, not HHVM, to execute composer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having to run composer with PHP (not hhvm) has been the case since hhvm 4.0. I'd personally not mention it anymore.
guides/hack/21-XHP/07-basic-usage.md
Outdated
@@ -51,9 +101,11 @@ class defines what attributes are available to objects of that class: | |||
echo <input type="button" name="submit" value="OK" />; | |||
``` | |||
|
|||
Here the `:input` class has the attributes `type`, `name` and `value` as part of its class properties. | |||
Here the `input` class has the attributes `type`, `name` and `value` as part of its class properties. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing that you changed here, but attributes are not class properties.
They are actually values in a dict in Core\element
.
I'd drop as part of its class properties.
from the text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's a bit of nuance here: they are not properties, but the typechecker considers them to be properties
Runtime: ->:foo
is ->getAttribute('foo')
Typechecker: ->:foo
is retrieving the instance property :foo
guides/hack/21-XHP/07-basic-usage.md
Outdated
|
||
Some attributes are required, and XHP will throw an error if you use an XHP object with a required attribute but without the attribute. | ||
Some attributes are required, and XHP will throw an error if you use an XHP object with a required attribute but without the attribute. Depending on the exact HHVM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The typechecker error does not depend on your hhvm version (provided you are using 4.8.0 or greater). It is toggled by the check_xhp_attribute
.hhconfig setting.
Might be nice to note that the runtime will not throw at the point of the xhp creation expression, but during the rendering process.
It might be nice to explain @lateinit
(the little brother of @required
) too (added in hhvm 4.8.0). @lateinit
and @required
are identical, other than the fact that not assigning to a @lateinit
attribute is not a typechecker error when check_xhp_attribute
is turned on. A full explanation can be found here blog post for hhvm 4.8.0
guides/hack/21-XHP/16-extending.md
Outdated
|
||
You can use the standard regex operators `*` (zero or more), `+` (one or more) `|` (this or that) when declaring your children. | ||
If you don't use the child validation trait, then your class can have any | ||
children. If you try to add a child to an object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following comment is a bit pedantic and this may not warrant such an explanation.
If you don't use the child validation trait, you effectively "inherit" the child rules of the primitive enclosing your children in your rendered output.
// This class effectively has
// XHPChild\any_number_of(XHPChild\of_type<li>())
// as it's child declaration, since that is what <ul> has.
final xhp class my_ul extends x\element {
protected async function renderAsync(): Awaitable<node> {
return <ul>{$this->getChildren()}</ul>;
}
}
`XHPHTMLHelpers` has two methods to add a class name to the `class` attribute of | ||
an XHP object. `addClass` takes a `string` and appends that `string` to the | ||
`class` attribute; `conditionClass` takes a `bool` and a `string`, and only | ||
appends that `string` to the `class` attribute if the `bool` is `true`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will also add a space between the previously last classname in the classlist and the string you just added.
guides/hack/21-XHP/19-migration.md
Outdated
|
||
```Hack | ||
function render_component($text, $uri) { | ||
async function render_component($text, $uri) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is missing the Awaitable<string>
return typehint.
guides/hack/21-XHP/19-migration.md
Outdated
namespace ui; | ||
|
||
class link extends x\element { | ||
attribute Uri $uri @required; // Assume class Uri |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
XHP attributes do not have a $
preceeding the attribute name.
- attribute Uri $uri @required
+ attribute Uri uri @required
guides/hack/21-XHP/19-migration.md
Outdated
namespace ui; | ||
|
||
class link extends x\element { | ||
attribute Uri $uri @required; // Assume class Uri | ||
attribute string $text @required; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL that you can have more than one attributes declaration in a class.
I thought that you had to put all attributes in one declaration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have found the source of my confusion. In the hhvm 4.3.0 blog post the rules for multiple children
and category
declarations changed. Multiple attribute
declarations are still allowed.
// Make sure that attributes are transferred automatically when rendering. | ||
use XHPHelpers; | ||
protected function render(): \XHPRoot { | ||
use XHPAttributeClobbering_DEPRECATED; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example should probably be replaced with an example of attribute splat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you use attribute splat to get similar behavior? <div {...$this}>
? Does it ignore attributes that exist on $this
but don't exist on <div>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spread operator expands to all attributes which are valid on the tag to which the attributes are being transfered to. If the attribute doesn't have a value null
or unset
in the transfered from tag, the attribute is not transfered. The relative order of the attribute locations in the transfered to tag does matter. Later occurrences of an attribute override previous ones. For an example see 3.24.0 blog post.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I recommend using attribute inheritance (attribute :some:other:element;
) + the splat operator?
I assumed we'd want to move away from attribute inheritance along with attribute clobbering (I removed the inheritance section and said we recommend explicitly listing attributes in the clobbering section), but maybe that's incorrect...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AIUI attribute inheritance is likely staying for as long as attributes remaining a distinct thing to properties (then interfaces can be used) - for now, inheritance + splat seems like the right mix.
Requiring an explicit list will get very verbose - for example, class
, and style
are likely to be supported pretty much everywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand the comment about how interfaces could be used when attributes are backed by properties instead of an associative container. Would you mind clarifying that bit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... TIL that interfaces can't declare properties. I was thinking of this, but it's invalid:
interface Foo {
public string $bar;
}
class HerpDerp implements Foo {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would traits fit the use case? I know they are sometimes frowned upon, but they seems to be up for the task.
I am still a bit unsure about how this could recreate the splat operator. Misread, splat isn't going away. The implementation would need to change to not violate typesafety.
Properties can have XHPUnsafeAttribute, so that will be a challenge to overcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wouldn't affect the splat operator, it would remove the need for attribute inheritance as a distinct concept though.
disable_xhp_element_mangling=true | ||
enable_xhp_class_modifier=true | ||
disable_xhp_children_declarations=true | ||
check_xhp_attribute = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps we should add support for a prepended/default hhconfig for examples in the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't really matter if we wait until these are on by default, I can just remove that commit then.
|
||
@@ extending-examples/basic.inc.php @@ | ||
@@ extending-examples/basic.php @@ | ||
|
||
**Historical note:** Before XHP namespace support (in XHP-Lib v3), use | ||
`class :intro_plain_str` instead of `xhp class intro_plain_str` (no `xhp` | ||
keyword, but requires a `:` prefix in the class name). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we probably want an fb span here saying that (for now) we're also using the old syntax, and no namespace support
From the new commits, these are probably worth reviewing: |
only review the last commit (16b4459), the rest is from #898
notes:
preview: http://35.162.222.227:8080/hack/XHP/introduction
(will try to keep it up for a few days; sorry no https, you might get MITMed and get evil documentation instead)