Skip to content

Plugin Interface Review

Jump to bottom
Dominic Chambers edited this page Jan 12, 2015 · 5 revisions

The OrderedPlugin interface

This is not a final plug-in interface itself, but is extended by some of the other plug-in interfaces where plug-in ordering is important. It seems like a sensible design since it allows plug-ins to fully describe their ordering requirements in terms of other plug-ins they are aware of.

The CommandPlugin interface

This is a good, solid interface. There have been some objections as to the way in which we make argument parsing available though:

  1. It requires that ArgsParsingCommandPlugin is extended, yet this may not be immediately obvious to people, or may feel like something that isn't properly supported.
  2. It requires developers to use the JSAP library, which is poorly documented and doesn't have a fluent interface, even though that would feel more natural.

At a minimum, it would make sense to make a very big mention of ArgsParsingCommandPlugin in the CommandPlugin docs, and to provide examples of the three types of argument you can document with JSAP, to make up for it's documentation. It might also make sense to rename ArgsParsingCommandPlugin to JSAPCommandPlugin or JSAPArgsParsingCommandPlugin, to make it more obvious that it's reliant on a particular library.

The ModelObserverPlugin interface

This interface has no methods other than those defined within Plugin, and is merely used as a marker, to denote intent. There is therefore no more to say about it.

The TagHandlerPlugin interface

This interface mostly makes sense. The getDependentContentPluginRequestPrefixes() seems awkward though, and as far as I can tell can be determined automatically from getGeneratedContentPaths(). Assuming this is the case, this method should be removed, and a utility method provided to calculate it instead, for example:

ContentPathPrefixLocator.getDependentContentPluginRequestPrefixes(List<String> contentPaths)

Another potential concern is that tag-handlers are presently only able to handle tags with attributes, and can't handle tags containing other tags. This might easily become a problem later, and could be solved by instead passing the tag contents as a string, and providing a utility to convert to a map of attributes where this is sufficient.

The ContentPlugin interface

This interface has a couple of dubious methods:

  • getCompositeGroupName() seems wrong since it breaks the interface-segregation principle, given that not all plug-ins are composites.
  • getContentPathParser() also seems wrong since it may limit longevity if we discover some future content plug-ins that can't parse their arguments using ContentPathParser, and in any case is awkward for plug-ins that don't have any complex parsing to perform.

Two new interfaces could be created (e.g. CompositeContentPlugin and RoutableContentPlugin) that add these methods in for those that want to implement them.

The MinifierPlugin interface

This interface is almost certainly incorrect, but can't be fixed until we try to add source-map support to BladeRunnerJS. Since that isn't an immediate concern for us, the interface should probably be marked as beta or unstable so that we can feel free to change it later, post 1.0.

The Asset plug-in interfaces (AssetPlugin, AssetLocationPlugin & RequirePlugin)

At present, asset-discovery happens like this:

  1. The model discovers the asset-containers (e.g. libraries, aspects, blades, bladesets, etc).
  2. The various AssetLocationPlugin instances are given the chance to describe the set of asset-locations within an asset-container, or to augment the set of asset-locations described by some other AssetLocationPlugin instance.
  3. The various AssetPlugin instances are given the chance to create assets for the various files within each asset-location.
  4. If a linked-asset depends on a plug-in style require-path, for which a RequirePlugin exists, then the RequirePlugin is given the opportunity of creating an asset for that require-path.

There a number of problems with the current arrangement:

  1. The AssetLocation interface is a bad abstraction that can only barely be made to work for the three AssetLocationPlugin implementations we have so far.
  2. The AssetPlugin interface requires there be a one-to-one correspondence between files and logical assets, which isn't always the case.
  3. The RequirePlugin inteface only exists, and has to allow on-demand asset discovery because, the AssetPlugin interface is incorrect.
  4. We now require three separate plug-in interfaces just to perform the asset-discovery dance.

The first problem can solved by getting rid of the concept of asset-locations, and instead using source-modules to model them in cases where they are needed; whereas the AssetLocation interface isn't quite right for any of our use cases, source-modules provide a much more flexible way of dividing up the asset discovery space, and describing the graph of dependencies for assets within those spaces.

The second and third problems can be solved by allowing all assets within a directory to be discovered by asset-plugins. Finally, the fourth problem disappears as a result of the previous two amendments, causing us to end up with a single new AssetPlugin interface to replace the current three interfaces:

void discoverAssets(AssetContainer assetContainer, MemoizedFile dir, String requirePrefix,
    List<Asset> implicitDependencies, AssetDiscoveryInitiator assetDiscoveryInitiator);

With this new interface, asset discovery now looks like this:

  1. The model discovers the asset-containers (e.g. libraries, aspects, blades, bladesets, etc).
  2. The various AssetPlugin instances are free to define any discovered assets within the root asset-container directory, and to cause a new round of discovery to occur within any discovered asset-directories.
  3. Step 2 repeats until all discovery ceases.

New rounds of discovery will be triggered with the help of the AssetDiscoveryInitiator class, itself having a similar interface to AssetPlugin:

void registerAsset(Asset asset);
void discoverFurtherAssets(MemoizedFile dir, String requirePrefix, List<String> implicitDependencies);

By having requirePrefix and implicitDependencies parameters, the namespace and implicit dependencies of any newly discovered assets can be controlled. Additionally, the AssetDiscoveryInitiator class will ensure that asset directories are not traversed multiple times, and that assets can only be registered once.

Clone this wiki locally