angular / angular.js

AngularJS - HTML enhanced for web apps!
https://angularjs.org
MIT License
58.8k stars 27.48k forks source link

Add check to addDirective fuction to alert/stop adding same directive #12387

Closed OpenSiteMobile closed 7 years ago

OpenSiteMobile commented 9 years ago

This is an informal feature request. The "addDirective" function under $CompileProvider -> "applyDirectivesToNode" doesn't check to see if the next directive being added, has already been previously added. And as far as I can tell, it doesn't seem to make a difference for the more typical running of AngularJS, with addtional modules all being loaded at page load. The directives apparently always get written to the same index anyway, so no bad things happen. So it isn't a bug or anything.

The problem arises when you load AngularJS style modules after page load. So dynamically loading some now standard modules will try to add the same directive twice (sometime via sub modules), and you get a "mutilple directive" error because of it. And yes this isn't probably something a lot of people are doing, even though maybe they should. Anyway, the fix is pretty simple:

    function addDirective(tDirectives, tAdded_directives, name, location, maxPriority, ignoreDirective, startAttrName,
                          endAttrName) {
      if (name === ignoreDirective) return null;
      var match = null;
      if (hasDirectives.hasOwnProperty(name)) {
        for (var directive, directives = $injector.get(name + Suffix),
            i = 0, ii = directives.length; i < ii; i++) {
          try {
            directive = directives[i];
            if ((maxPriority === undefined || maxPriority > directive.priority) &&
                 directive.restrict.indexOf(location) != -1) {
              if (startAttrName) {
                directive = inherit(directive, {$$start: startAttrName, $$end: endAttrName});
              }

              if (!tAdded_directives[directive.name]) {

                tDirectives.push(directive);
                match = directive;

                // Record as an object
                tAdded_directives[directive.name] = true;

              } else {
                console.log('Warning: already assigned directive: ' + directive.name + ', (ref. directive: ' + name + ')');
              }
            }
          } catch (e) { $exceptionHandler(e); }
        }
      }
      return match;
    }

The added code stops the error and stops the multiple addition of directives. It also warns that it happened, but this isn't important since everything runs fine anyway.

This would however require a new variable like added_directives = {}; being added to the collectDirectives function to gather the directive names as they are collected.

And I understand that this might not be a feature that will ever be added, but it took days to track down what was going on. The error message is correct, and even tells you it is the same directive. So I wanted to document it somehow so others who might come across this, won't have to go through the same grinding process trying to figure out what the heck is going on.

Narretz commented 9 years ago

If you can provide a PR with this change + tests, we can take a better look at what this means for compiling.

gkalpak commented 7 years ago

Closing since we haven't got any feedback from the OP. In addition, it is not exactly clear what the usecase is and the proposed fix will not work for directives with the same name (which is a perfectly valid usecase).