Skip to content
This repository has been archived by the owner on Apr 1, 2024. It is now read-only.

Namespaces/aliasing #64

Closed
RdeWilde opened this issue May 15, 2014 · 43 comments · Fixed by #227
Closed

Namespaces/aliasing #64

RdeWilde opened this issue May 15, 2014 · 43 comments · Fixed by #227
Assignees

Comments

@RdeWilde
Copy link

Implementing a alias to the current prefix '.' would be very efficient. I'm not sure how clean this would look. Alternative could be leaving out the prefix works like an alias to the parents prefix, but this would require to prefix the current default elements (when the parent is not).

<prefix:element>
   <.:header>
      <small>text</small>
   </>
</>

or

<prefix:element>
   <header>            <!-- eq prefix:header -->
      <html:small>text</html:small>
   </>
</>
@fredemmott
Copy link
Contributor

I'd love a more generic aliasing system - something like 'use' semantics; there's been some brief mentions of namespace support, which could also solve this.

@RdeWilde
Copy link
Author

Any downsides for the 'use' case? :)

@Swahvay
Copy link
Contributor

Swahvay commented May 15, 2014

Hmmmm. The problem with <.:header> syntax is that it's contextual. What happens when you do $xhp->appendChild(<.:header />);? Just moving an element to another parent could change its namespace.

I think the better solution is figuring out how to implement true namespace support into XHP. We could treat XHP's faux namespacing like real namespaces, so <ui:button> would just translate into new ui\xhp_button, but this poses two problems:

  1. What do we do with the core XHP and HTML elements? Do we require their namespace be used in every file that uses XHP? That seems annoying.
  2. How do we reference global XHP elements? <\ui:button /> seems like a really weird syntax. So does <:ui:button>.

I'm not entirely sure what the best solution here is.

@RdeWilde
Copy link
Author

I understand the problem with the '.'-reference, this is indeed not an option.

I thought fredemmott was referencing to the real PHP namespace aliases to be used as prefix?

namespace Main {
   use Other as o;

   echo 
      <container>        // Would replace as Container (or the xhp__ replacement)
        <o:element />    // Would replace as \\Other\Element (or the xhp__ replacement)
      </container>; 
}

@lattwood
Copy link

@Swahvay,

You could only require a leading colon if you have imported or defined an element with the same identifier.

@Swahvay
Copy link
Contributor

Swahvay commented Sep 23, 2014

It gets significantly harder to parse and translate to standard PHP, if we have to keep track of imported files and their parsed XHP (read: basically not an option). What I mean by they is, there's no way when looking at a file to know if there are imported classes that may have the same name as the one you're creating.

@fredemmott
Copy link
Contributor

We're going to implement standard namespace semantics, with ':' being equivalent to ''. Examples and exceptions at https://gist.github.com/fredemmott/1e65842fadb9e66e2359

As a short term step to help migration, if <foo:bar /> does not resolve, HHVM will look for <:foo:bar /> - if it exists, it will use it, but log E_DEPRECATED.

Prefix aliasing will be done via 'use', just like any other namespaced class.

@fredemmott fredemmott self-assigned this Apr 3, 2015
@fredemmott fredemmott added the v2.1 label Apr 3, 2015
@TJ09
Copy link
Contributor

TJ09 commented Apr 3, 2015

Does this mean that XHP class names should look like PHP class names (in which there's no requirement, but the most common convention is camel case instead of dashes)? Or will names be the same as now (lowercase with dashes), and the recommendation will be to alias the namespace to something short and lowercased as well?

At the same time, React already uses capitalized/camelCase component names, so maybe it's not that bad of a change.

@fredemmott
Copy link
Contributor

The built-in classes are keeping the current names, and this is likely to stay (eg x:composable-element, not x:ComposableElement). I expect this will lead to continued use of the current convention.

We're not going to explicitly recommend aliasing, or discourage it. The main reason this got prioritized was:

  • Hack requires globally unique class names
  • This includes unit test classes, both for your project, and any libraries you depend on
  • The easiest way to do this is to add namespace declarations
  • XHP really doesn't work well with that at the moment :p

Being able to alias prefixes is a nice side-effect :)

@ghost
Copy link

ghost commented Aug 4, 2015

Thank you for reporting this issue and appreciate your patience. We've notified the core team for an update on this issue. We're looking for a response within the next 30 days or the issue may be closed.

@fredemmott
Copy link
Contributor

no thanks FBBot.

@TJ09
Copy link
Contributor

TJ09 commented Aug 4, 2015

Since the bot brought it up...what is the status of this? Right now it's difficult to use XHP from namespaced classes, because hack doesn't look in the global namespace for HTML, resulting in errors like "Unbound name: NonGlobalNamespace:div."

@fredemmott
Copy link
Contributor

Still planned, but no ETA and not a current priority. Most likely to happen at a hackathon sometime.

@fredemmott fredemmott changed the title Alias for current (parent) prefix Namespaces/aliasing Aug 18, 2015
@fredemmott
Copy link
Contributor

I'll see if we can put this up on bountysource. Will need changes to both XHP-Lib and HHVM.

@GerardSmit
Copy link

Removed all the namespaces in my project because of this issue.
Is there any news on this issue? Because next month this issue will be 2 years old... 🎉

@fredemmott
Copy link
Contributor

It's still something we'd like to happen, and I'll happily review pull requests (both for XHP-Lib and HHVM parts). Unfortunately it's something I'd have to do on my own time instead of for Facebook :(

@TJ09
Copy link
Contributor

TJ09 commented Apr 8, 2016

As a simpler (but less ideal) solution, can hh just be taught to know that XHP classes are allowed to fall back to the global namespace (ie updating hack server to match the existing runtime behavior)?

@fredemmott
Copy link
Contributor

We don't want to do that as that means that we can't do it correctly in the future without a major BC break.

@fredemmott
Copy link
Contributor

cc @sgolemon given https://github.com/phplang/xhp

@sgolemon
Copy link

Funnily enough, I was just working on this problem as well for the PHP version of XHP.

My thought was something along these lines:

<?php

use :foo:bar as baz;

class :foo:bar:qux { ... }

echo <baz:qux attr="val" />; // Same as invoking <foo:bar:qux attr="val" />

The idea being that we do reuse the same basic semantics as regular PHP namespaces, but they occupy a separate space. e.g. foo\bar is a separate namespace than foo:bar)

In terms of implementation, what I have in my proof-of-concept patch is that a use statement adds a lookup to a per-file map (since PHP namespaces are file-scoped), and any XHP start tag checks the leading segment of its name against the map, to expand as it parses.

What this approach doesn't accommodate is tags which don't want to be expanded:

class :foo:bar:baz {}
class :qux:bling {}

use :foo:bar as qux;

echo <qux:bling />;  // My current patch over-expands this and gets undefined class error

I'm not sure that necessarily needs to be worried about, it smells like terrible code to me, and it can be dealt with by allowing root namespace modifying (e.g. <:qux:bling /> which is a bit anti-xhtml, but not terrible.

@jesseschalken
Copy link

The idea being that we do reuse the same basic semantics as regular PHP namespaces, but they occupy a separate space. e.g. foo\bar is a separate namespace than foo:bar)

So would this not be allowed?

namespace Foo\Bar {
  class :baz:boo {}
}

@fredemmott
Copy link
Contributor

fredemmott commented Aug 16, 2016

<?php

namespace Foo;

var_dump(get_class(<div />));

What would this output?

@sgolemon
Copy link

@jesseschalken I would say "Allowed, but not a good idea." Because now you have a tag sitting inside an effectively incompatible PHP namespace. How would you access that from a foreign file? <Foo\Bar\baz:boo /> ?

For that, we'd need to fully integrate the two namespace types, and there's enough differences between them that it feels like shoving a square peg into a round hole. If someone has a clean idea for how that'd look, I'm game, but I don't like any of the obvious routes.

@fredemmott string(7) "xhp_div" That is, the PHP namespace is irrelevant in this case.

@fredemmott
Copy link
Contributor

Is this equivalent to not actually namespacing them, but:

  • allowing prefix aliasing
  • adding implicit root lookup?

@sgolemon
Copy link

@fredemmott Yes, in the sense that XHP's current namespaces aren't really namespaces. They're just labels that happen to allow colons and hyphens in them. This adds use semantics while leaving them still not-really-namespaces.

@jesseschalken
Copy link

How would you access that from a foreign file? <Foo\Bar\baz:boo /> ?

Yeah. I don't see what's wrong with that?

Yes, in the sense that XHP's current namespaces aren't really namespaces. They're just labels that happen to allow colons and hyphens in them.

Before PHP introduced namespaces people were already using _, and the \, namespace and use features were added on top. No support for _ as a namespace separator was ever added.

I think it would be most straightforward and less surprising to PHP devs if XHP took the same route, enabled compatibility with PHP namespaces, and left : and - to function as they currently do.

You would end up with:

namespace Foo {
  use :ul, :li, :fb:thing;
  class :baz:qux {}
  echo <ul>
    <li>Hello</li>
    <li>World</li>
    <li>
      <Bar\boo />
      <baz:qux />
      <fb:thing />
    </li>
  </ul>;
}
namespace Foo\Bar {
  class :boo {}
}
namespace {
  use Foo\Bar\:boo as :bob:boo;

  echo <div>
    <Foo\baz:qux>
      <bob:boo />
    </Foo\baz:qux>
  </div>;
}

With the real class names as:

namespace Foo {
  use xhp_ul, xhp_li, xhp_fb__thing;
  class xhp_baz__qux {}
  echo <xhp_ul>
    <xhp_li>Hello</xhp_li>
    <xhp_li>World</xhp_li>
    <xhp_li>
      <Bar\xhp_boo />
      <xhp_baz__qux />
      <xhp_fb__thing />
    </xhp_li>
  </xhp_ul>;
}
namespace Foo\Bar {
  class xhp_boo {}
}
namespace {
  use Foo\Bar\xhp_boo as xhp_bob__boo;

  echo <xhp_div>
    <Foo\xhp_baz__qux>
      <xhp_bob__boo />
    </Foo\xhp_baz__qux >
  </xhp_div>;
}

This looks fine IMO.

@TJ09
Copy link
Contributor

TJ09 commented Aug 17, 2016

I disagree that <Foo\baz:qux> looks fine. Having backslashes in the middle of my XML (-ish) tag names feels wrong. To bring up the underscore vs namespace separator, I'd say mixing those doesn't work either; you can have vendor_product_class or \Vendor\Product\Class, but shouldn't have Vendor\product_class.

I think use semantics ("prefix aliasing + implicit root lookup") provide everything I'd want in XHP namespacing, and it solves the main reason this issue got prioritized in the first place ("Hack requires globally unique class names" -> I can name my components :vendor:product:thing instead of :ui:thing and use :vendor:product as ui). I can't think of a case where I'd need to have an XHP component in the same namespace as some non-XHP class.

@jesseschalken
Copy link

jesseschalken commented Aug 17, 2016

I disagree that <Foo\baz:qux> looks fine. Having backslashes in the middle of my XML (-ish) tag names feels wrong.

Having a completely different namespacing system running in parallel to PHP's existing namespaces just to avoid putting a backslash inside a tag name definitely feels wronger.

It's the same with React in JavaScript where, apart from modules, . is used as a namespace and <Foo.ReactComponent /> works fine.

To bring up the underscore vs namespace separator, I'd say mixing those doesn't work either; you can have vendor_product_class or \Vendor\Product\Class, but not Vendor\product_class.

Yes you can. This code works fine.

namespace Foo {
  class Bar_Baz {}
}

I don't see why you'd do it, but it works fine.

I think use semantics ("prefix aliasing + implicit root lookup") provide everything I'd want in XHP namespacing, and it solves the main reason this issue got prioritized in the first place ("Hack requires globally unique class names" -> I can name my components :vendor:product:thing instead of :ui:thing and use :vendor:product as ui).

With interop with PHP namespaces you can do

namespace Vendor\Product\ui;
class :thing {}
namespace Blah;
use Vendor\Product\ui;
echo <ui\thing />;

or even

namespace Blah;
use Vendor\Product\ui\:thing;
echo <thing />;

Just build in your existing namespace hierarchy.

I can't think of a case where I'd need to have an XHP component in the same namespace as some non-XHP class.

I write React components in the same file as standard JavaScript classes all the time. In fact, I don't even have a JavaScript file that contains a React component without a class, function, constant, variable or type definition in the same file. Why should XHP stop me from doing the same?

Projects already have their own namespaces like Illuminate\Pagination. Why shouldn't they be able to declare Illuminate\Pagination\my-xhp-element in the same namespace, even in the same file?

@sgolemon
Copy link

sgolemon commented Aug 17, 2016

Okay, thinking of the colon as having no significance... Yes, I agree with you. And yes, there is certainly precedent when compared to the underscore's role pre PHP-5.3.

I still don't like the look of a backslash in an XHP tag, but I could get over that. (I never liked the backslash as a namespace separator to begin with)

For that route we'd just need backslashes respected in tagnames (along with some mangling updates)

In terms of my goals (which I realized I only put in the FB post, but not here), I was hoping to make XHP tags more composer friendly, and this does accomplish that. A library package might look like:

<?php

namespace PhpLang\XhpLib\ui;

class :widget {...}
class :woggle:toggle {...}

Could be autoloaded with the usual "psr-4": { "PhpLang\\XhpLib\\": "src/" } kind of composer mappings and be used by a project via:

<?php
use \PhpLang\XhpLib\ui;

echo <ui\widget><ui\woggle:toggle/></ui\widget>;

Functionally speaking it won't be hard to get this working, and since it'd use PHP's existing name resolution mechanisms there would be less to get wrong. I like getting more benefit for less work, so that might be enough for me to swallow something as ugly as </ui\widget> being valid syntax.

@fredemmott is anyone on your end interested in weighing in? Happy to drop by and freeload some lunch to talk it out if someone with an opinion is available this week (I'm out of the country next week and into the following).

@fredemmott
Copy link
Contributor

I'm tight on time this week and can't really look in detail (especially comparing advantages/disadvantages to the previous conclusion). I can leave notes here next week for when you're back and talk after?

Also, @Swahvay: ping :)

@Swahvay
Copy link
Contributor

Swahvay commented Aug 18, 2016

@sgolemon, I agree with basically the entirety of your last comment. The syntax looks hideous, but I always thought the \ namespace separator was hideous to begin with too.

I keep thinking of the old adage, "code is written to be read," and although it's not pretty

<?php
use \PhpLang\XhpLib\ui;

echo <ui\widget><ui\woggle:toggle/></ui\widget>;

is pretty clear what's going on.

I sort of wish we could go the route of treating colons in XHP like a namespace separator, so <ui:widget> and <ui\widget> would be equivalent, but I think that would just cause more confusion that it's worth.

As I see it, the only remaining confusion from adding namespaces to XHP with the standard separator will be, "What are colons for in XHP classes?"

@fredemmott
Copy link
Contributor

fredemmott commented Aug 18, 2016

That reminded me... this I really dislike:

<?php
namespace foo;

echo <somethinginfoo><\div>herp derp</\div></somethinginfoo>;

"What are colons for in XHP classes?"

Just another valid token for class name (sort-of), make it deprecated though?

@fredemmott
Copy link
Contributor

@sgolemon I was hoping to make XHP tags more composer friendly

I guess you mean PSR-n? They're supported via the classmap approach, and the Hack xhp-lib itself uses that - the standalone autoload.php is just for people who choose not to use composer.

@Swahvay
Copy link
Contributor

Swahvay commented Aug 18, 2016

Just another valid token for class name (sort-of), make it deprecated though?

That would really be unfortunate, since the X in XHP is for XML, which uses the colons. Plus for people (like myself) who don't use PHP namespaces in their projects, I find the faux namespaces of XHP very nice.

Also agree that <\div>herp derp</\div> is disgusting. :/

@sgolemon
Copy link

@Swahvay

I sort of wish we could go the route of treating colons in XHP like a namespace separator, so ui:widget and <ui\widget> would be equivalent, but I think that would just cause more confusion that it's worth.

The main issue there, in my mind, is class declarations:

class :foo:bar { }
class \foo\bar { } /* This version makes no sense in terms of PHP namespaces, so neither would the previous */

Maybe we expand classes+namespaces to allow syntax like this as well, but that's a separate dimension to the problem.

@fredemmott

@sgolemon I was hoping to make XHP tags more composer friendly
@fredemmott I guess you mean PSR-n? They're supported via the classmap approach, and the Hack xhp-lib itself uses that - the standalone autoload.php is just for people who choose not to use composer.

Classmap is a bit on the klunky side, actually. But no, that's not so much the problem; I've added a class loader: https://github.com/phplang/xhp-lib/blob/master/src/ClassLoader.php which is basically PSR-4, but using colons instead of backslashes. The loader gets hooked in an {"autoload":{"files":[]}} include:

(new PhpLang\XhpLib\ClassLoader([
  ':view' => __DIR__ . '/../view/',
])->register();

Not quite first-class, but it works. That said, I'd prefer to have a meaningful way to make XHP fit into PHP namespaces, hence involvement in this thread.

echo <somethinginfoo><\div>herp derp</\div></somethinginfoo>;

I think I just threw up in my mouth a little.

@fredemmott
Copy link
Contributor

Where are we at? as far as I can tell:

  • most of us agree that backslashes don't work nicely with xml-like syntax
  • we don't really have much agreement beyond there

I still think that https://gist.github.com/fredemmott/1e65842fadb9e66e2359 is the best approach; in addition to the reasons in that gist, as xhp classes must either be in the root namespace or only have a single leading colon, it also solves the PSR-4 friendliness issue.

@fredemmott
Copy link
Contributor

I'm hoping to get to this around July, maybe a bit later. This is getting more and more painful.

The main delay is that currently, this involves changing:

  • HHVM's parser
  • the main Hack parser
  • Hacks full fidelity parser

In a few months, we should be at a point where only the FFP needs changing.

@fredemmott
Copy link
Contributor

Work-in-progress pull request at facebook/hhvm#8578 - this led to us finding issues with it.

Current status and open questions at https://gist.github.com/fredemmott/7316287e3fb9cc43b312b3e51c155ba5

@fredemmott fredemmott self-assigned this Feb 19, 2020
@fredemmott
Copy link
Contributor

fredemmott commented Feb 19, 2020

As of tomorrow's nightly builds, I believe this is usable, but with codemods/migrations required, and non-default hhconfig and hhvm options.

In particular, with these options enabled:

#216 contains xhp-lib tests of this, though needs extending to cover the runtime

The majority of issues labelled 'v4' must be completed before we can flip the defaults and ship a new xhp-lib.

codemods/migrations are being added to HHAST to do the parts that can be done in isolation, e.g. hhvm/hhast#267 and hhvm/hhast#268

@fredemmott
Copy link
Contributor

ext_factparse is currently unable to handle the new syntax, so autoloading is broken; facebook/hhvm#8661 to fix

@fredemmott
Copy link
Contributor

Think we're done, except for flipping defaults and banning 'children' declarations.

Doing a final internal check for feedback.

@fredemmott
Copy link
Contributor

Also, except for the final one, that's unlikely to be the new API - there will be more manual work involved e.g. instead of simply replacing - with _; that's the minimum to check functionality.

@fredemmott
Copy link
Contributor

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

Successfully merging a pull request may close this issue.

8 participants