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

Add node processing hook to $compile public API. #15270

Open
Aaron-Pool opened this issue Oct 14, 2016 · 13 comments
Open

Add node processing hook to $compile public API. #15270

Aaron-Pool opened this issue Oct 14, 2016 · 13 comments

Comments

@Aaron-Pool
Copy link

Aaron-Pool commented Oct 14, 2016

Do you want to request a feature or report a bug?

Feature

What is the current behavior?

I have no way of modifying a node before the majority of compile functionality takes place (that I've found, and yes, I've read the source :) ).

Suppose I want all components named mySubmenu to also have the directive genericStateListener applied to them. I've separated out a lot of ui-router state based functionality into it's own directive, in keeping with the separation of concerns principle. I don't want a bunch of generic state handling logic clogging up my component controller. Additionally, I may want to use this state logic on other components that are concerned with state transitions.

Well, to do this, I have two options (that I know of):

  1. I could place the directive genericStateListener in every place I use mySubmenu. Well, that's annoying :)
  2. I could wrap my template for mySubmenu in a block which has the genericStateListener directive. This is workable, but it bothers me that someone else viewing my code needs to go into my template to understand that this component has state listening functionality. Additionally, if I want to interact with the genericStateListener directive controller, I cannot require it, because it's BELOW me now.

Now the situation becomes even MORE complex (but also more simple) in the instance that, perhaps I don't want to be tied to a particular functionality/directive for listening to states. I want to abstract it further. In fact, I just want to be able to put an isStateListener field on my component. Then, by overriding the .component decorator, I can add an annotated field $stateListener every time I see this field on my options object. GREAT I now have a way to check if the user wants to have stateListener functionality on the component I'm compiling! These are the kinds of things I can EASILY do already in angular's current state. There's only one problem, even once I get said attribute on the final directive object... I can't do anything with it.

There's no way of modifying a node prior to the start of the compilation process (before directives are collected). By the time I can add functionality to the $compile process, directives have already been collected. This seriously hampers extensibility of the component object.

What is the expected behavior?

I would expect there to be a publicly accessible function to be run somewhere in the collectDirective function, like this:

  switch (nodeType) {
        case NODE_TYPE_ELEMENT: /* Element */
          // use the node name: <directive>
          runNodeTransforms(nodeName_(node),node); // function exposed on $compileProvider
          addDirective(directives,
              directiveNormalize(nodeName_(node)), 'E', maxPriority, ignoreDirective);

          // iterate over the attributes
          for (var attr, name, nName, ngAttrName, value, isNgAttr, nAttrs = node.attributes,
                   j = 0, jj = nAttrs && nAttrs.length; j < jj; j++) {
            var attrStartName = false;
            var attrEndName = false;

This would enable me to implement so much additional functionality in a clean, concise, and for the sake of other developers on my project, abstractly. For example, suppose I'm using https://github.com/tenphi/angular-bem, and I want all components to also have a block
directive on them for simple, clean BEM notation. I could just do this:

function componentDecoratorConfig($compileProvider) {
        "ngInject";
        var oldProvider = $compileProvider.$get,
            oldComponent = $compileProvider.component;

        $compileProvider.component = registerComponent;

        function registerComponent(name, options) {
            options.$isComponent = true;
            return oldComponent.bind($compileProvider)(name, options);
        }

       $compileProvider.addNodeTransform(function(name, node) {
            if($injector.has(name + 'Directive') 
                && $injector.get(name + 'Directive').controller.$isComponent) {
                node.attr.set('block', name);
            }
            return node;
        });

And voila! It's done! I'll admit that there are some parts of the compilation process that I don't fully get, and maybe this will have unintended consequences that I don't understand, but it seems like a simple solution to a problem that provides extensibility that is more generic that traditional specific directive decorators.

There was a similar pull request done by @lgalfaso , here: #6781, it seemed to have gotten some good feedback, but it was pretty involved, and it looks like it got shelved.

I'd be glad to create a pull request, if I thought this functionality had any chance of being merged.

-Aaron

@gkalpak
Copy link
Member

gkalpak commented Oct 14, 2016

This is an interesting idea. #6781 is indeed more involved and probably not suitable for 1.x at this point, but a more "lightweight" alternative might be a useful addition.

Regarding the first usecase (i.e. adding a directive whenever another directive is used), it is possible today - although admittedly not in the most straight-forward of ways 😃

Basically, you can have a high-priority, terminal directive (which means it will stop the compilation), that attaches the extra attribute and the comtinues the compilation (for lower priorities):

.component('mySubmenu', { ... })
.directive('genericStateListener', function() { ... })
.directive('mySubmenu', function($compile) {
  // DDO
  return {
    restrict: 'E',
    priority: 2000,
    terminal: true,
    compile: function mySubmenuCompile(tElem) {
      tElem.attr('genericStateListener', '');
      var compiled = $compile(tElem, null, 2000);

      return function mySubmenuPostLink(scope) {
        compiled(scope);
      };
    }
  };
})

Demo

@Aaron-Pool
Copy link
Author

Aaron-Pool commented Oct 14, 2016

So, yeah, I was aware of that method, however, if instead I want to check for a condition, as opposed to a particular directive, it becomes more difficult. I actually think I figured out a way to, with the way the public API is already, but it was only after initial directive collection had taken place. That meant that to actually apply the directive, I'd have to recompile it, and if a bunch of directives satisfy said conditions, then I didn't want to call $compile on all of them. I thought that might be a pretty intense performance hit (not super well-versed in how performance-intensive $compile is, however, so that may be a silly issue to be concerned about).

@gkalpak
Copy link
Member

gkalpak commented Oct 14, 2016

Checking a condition should also be possible with the same technique:

.component('mySubmenu', { ..., $isStateListener: true })
.directive('genericStateListener', function() { ... })
.directive('mySubmenu', function($compile, $injector) {
  // DDO
  return {
    restrict: 'E',
    priority: 2000,
    terminal: true,
    compile: function mySubmenuCompile(tElem) {
      var dirs = $injector.get('mySubmenuDirective');
      var isStateListener = dirs.some(function (dir) {
        return dir.controller && dir.controller.$isStateListener;
      });

      if (isStateListener) tElem.attr('genericStateListener', '');
      tElem.attr('genericStateListener', '');
      var compiled = $compile(tElem, null, 2000);

      return function mySubmenuPostLink(scope) {
        compiled(scope);
      };
    }
  };
})

Obviously this is a very verbose and inconvenient technique and that is why I still thing your proposal sounds interesting. I am just pointing out what is possible today (in case someone needs it).

@Aaron-Pool
Copy link
Author

Sorry, when I said "check a condition", I meant check for a condition in ALL directives. The example you gave still only works to add a directive to all mySubmenu components, as opposed to ANY component with $isStateListener. Does that make more sense? Or am I still wrong?

@Aaron-Pool
Copy link
Author

Additionally, your solution still results in, essentially, a double compilation, right?

@gkalpak
Copy link
Member

gkalpak commented Oct 14, 2016

The example you gave still only works to add a directive to all mySubmenu components, as opposed to ANY component with $isStateListener.

True. You could device some hack for certain situations (e.g. monkey-patching .component() and add a duplicate directive for each component), but that will not work for other usecases (plus might have a slight impact of performance, especially if you have many component definitions).

ATM, I don't think there is a way to apply a directive to all elements.

Additionally, your solution still results in, essentially, a double compilation, right?

Not exactly. Since my directive is terminal, only directives with a priorirty >=2000 will be compiled and then, it will manually compile for directives with a priority <2000.

(BTW, double-compilation - in the sense of re-compiling an already fully compiled element - is not supported and may lead to all sorts or memory leaks and unexpected behavior.)

@Aaron-Pool
Copy link
Author

Mmm, ok, that's good to know. It also further enforces the idea that this could be a clean/simple way of doing something that's currently not available by other means, right? So, would a pull request be considered if I submitted one?

Or are we still just waiting on opinions from more of the angular team?

@gkalpak
Copy link
Member

gkalpak commented Oct 15, 2016

I would like to get more opinions. My main concern is that we generally try to avoid introducing features that are difficult or impossible to map to Angular 2, as that would make it much harder for people to migrate their apps. And I am not sure if something similar is possible in ng2 in a straight-forward way.

@gkalpak gkalpak modified the milestones: Purgatory, 1.5.x Oct 15, 2016
@Aaron-Pool
Copy link
Author

Aaron-Pool commented Oct 15, 2016

Oh, that totally makes sense. I guess I just assumed there was probably a way of doing this in ng2, since similar functionality had been sitting in the icebox for angular for a while.

@Aaron-Pool
Copy link
Author

Aaron-Pool commented Oct 16, 2016

There seems to be a parallel conversation happening on the ng2 repo: angular/angular#8785, though I would argue that this feature is slightly more overarching than JUST having the option to add directives to the host element within the component declaration, but that does seem to be the most common use case.

@Aaron-Pool
Copy link
Author

@gkalpak so, is the fact that this got added to the backlog milestone a good thing, or a bad thing?

@gkalpak
Copy link
Member

gkalpak commented Oct 20, 2016

@Aaron-Pool, it could be worse 😃 As discused above, it is better to wait and see which way Angular 2 goes. We can make a more informed decision then. So, let's keep an eye on angular/angular#8785.

@Aaron-Pool
Copy link
Author

Will do, thanks!

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

No branches or pull requests

2 participants