I think those diffs are basically fine, but I'd like to decouple Celerity maps from libphutil libraries more. I don't think we gain much by coupling them 1:1. Even in Phabricator, it might be nice to generate separate Phame and Phabricator maps, for example.

The term "module" is also sort of overloaded. At one point it meant a specific directory within a phutil library (e.g., phabricator/src/ is a library, and phabricator/src/applications/differential/controller/delete/ was a module. However, modules became pretty much meaningless once we got rid of __init__.php, a long time ago. I think they're still used inside libphutil itself, however.

Instead, the map file could just encode:

• The name of the map (e.g., "phabricator").
• The root of the map on disk, relative to the map file (e.g., "../../webroot/rsrc/").
• The existing package and resource data it records today.

Then we can load multiple maps at runtime, and change the resource URIs to include the map name. This would let us get rid of celerity.resource-path, which is pretty hacky.

I think the only limitation this imposes is that you can't package resources across libraries. I think that's 100% fine for now, and we can add some optional aggregate build step which builds packages across every loaded map in the future.

Does that seem reasonable? I think the module diffs are good, but they feel like half a step forward and half a step sideways, when we could just take a full step forward and decouple resource maps from libphutil libraries completely.

(This has the nice effect of not requiring stuff to be structured with src/ and webroot/ and such exactly, too, although we could make the CLI flags use those as defaults.)

The only thing with decoupling them completely is discovery; how does Celerity discover all of the relevant maps to load? Do we use the file finding class to search for all files called __celerity_resource_map__.php across all libphutil libraries that are loaded? That seems reasonably expensive.

For now, let's just say you have to symlink them all into some directory (phabricator/resources/maps/) or something like that? We could do some kind of autodiscovery later on, but that shouldn't be too much of a barrier for anyone doing custom development.

As discussed in IRC, I think providing more than 1:1 relationship at this stage complicates things (like auto-discovery) for some very theoretical use cases. The changes made in the diffs attached to this task actually do the heavy lifting of allowing Celerity to serve from multiple resource maps. Changing this in the future to pull from other locations (such as a resources/maps folder) is reasonably trivial to do if there's ever a concrete use case where we would benefit from a 1:* mapping.

@epriestley Just following up on this since I had to upgrade today and resolve conflicts (I currently have these patches applied to my install as I need them to run websites). Given that we haven't heard back from Facebook regarding this issue, is there anything blocking it's acceptance in upstream?

I don't want to accept the 1:1 version in the upstream. Although I don't have a strong, concrete argument for why it won't work, I think it's worse in principle in a lot of ways (tight coupling to libraries, singleton-ness) without having any advantages (except that it's already written), and particularly it's worse in a "not easy to move forward" kind of way. I'm fine with not-so-great solutions which have room to grow, but I think 1:1 is mostly a dead end and we'd have to back out of it -- at least somewhat -- to move to arbitrary numbers of maps. We already dead-ended once (with celerity.resource-path) and I don't want to dig out of that hole just to dig into a different one.

I'm going to provide two alternate changes, broadly:

• Remove celerity.resource-path.
• A non-1:1 multimap implementation, probably mostly based on D7753 / D7754.

I'm off for the night but I'll finish this up tomorrow, I think there's very little work left.

After D7877, I was able to:

• Add a small subclass of CelerityResourcesOnDisk to an external library to define resource and map locations.
• Add resources to an external library.
• After including the library, rebuild the map with bin/celerity map and see it update in the console output.
• Write a library View class which overrides getDefaultResourceSource().
• Use $this->requireResource() from within that View tree to require external resources by default.
• Have the whole thing work correctly when loaded; here's a screenshot of the library including a * { background: pink; } CSS resource:

Stuff that does not work yet, which I don't plan to make work for now:

• External resources can not currently depend on Phabricator symbols. This would be nice to have, but isn't hugely difficult to work around. It isn't relevant if you aren't using Phabricator chrome. I'd plan to implement it eventually, but probably not for a long long time without more compelling use cases.
• Packages can not package resources across libraries. I think this is of limited use and don't currently ever plan to implement it, as it seems like a lot of pain for very little gain right now.
• A few details, like "high priority behaviors" are still deep in Celerity and should probably move to CelerityResources. I'd plan to do this as need arises, I think there's nothing in there that's very important or hard to separate.
• There's no super-easy-mode, like a standard rsrc/ directory which we just assume to contain resources if it exists. For now, I don't think there's any need for this. There's a direct path to building it on this infrastructure later if we want.
• We could add a second custom map for users pretty easily now (e.g., for skinning/templating) but I don't plan to address that here. We can add it if it makes sense in the skinning/templating tasks.
• Some of the "virtual" / Phame resources stuff feels a little fuzzy, but I think it's separated from the normal stuff enough that this won't create issues.
• The requireResource() APIs on View and Controller feel like a step forward from require_celerity_resource, but not hugely great because they create a lot of lock-in for subclasses (you have to subclass for resource source, not for functionality). It might make sense to try to get these right by default: for example, a ResourceSource could have a method like isDefaultSource(), and then all Views/Controllers defined in library X could use the default resource source defined in library X, by default. That would probably get things right in 95% of cases, for free, without locking the meaning of subclassing. Not totally sure how/when to proceed here, but I'm inclined to leave it as-is for now and wait for feedback.