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

fix ($compile): keeps prototype properties for template URL directives #10926

Conversation

hannahhoward
Copy link
Contributor

currently, if a directive definition objects has prototype properties, such as a compile function, those are lost when compiled if the directive has a templateUrl function. Steps to reproduce:

directive("templateUrlDoesntWork", function() {
   var base = {
      compile: function() { 
         return function(scope, element, attrs) {
            scope.value = "Test"
         }
     }
  }
  inherited = Object.create(base)
  inherited.templateUrl = "test.html"
  return inherited;
}

The DDO's compile function will never be called. On the other hand the same version with a plain template works:

directive("templateWorks", function() {
   var base = {
      compile: function() { 
         return function(scope, element, attrs) {
            scope.value = "Test"
         }
     }
  }
  inherited = Object.create(base)
  inherited.template = "<p>{{value}}<p>"
  return inherited;
}

The DDO's compile function will be called.

This makes a difference for an increasing number of people particularly who are experimenting with ES6 classes in Angular 1.x.

@caitp
Copy link
Contributor

caitp commented Feb 1, 2015

i could see this being useful for typescript/coffeescript users, and it is a pretty small fix. lgtm, but wait for pete or someone else to signoff on it

@caitp
Copy link
Contributor

caitp commented Feb 1, 2015

Marking this for beta 3, but this should probably wait for beta 4. /cc @petebacondarwin

@petebacondarwin
Copy link
Contributor

LGTM - let's get this in next week. Thanks for taking a look @caitp and pinging me.

@lgalfaso
Copy link
Contributor

lgalfaso commented Feb 1, 2015

looks good, the only thing I would like to see is the same test using template. I know that this test would pass even without the PR, but having a test in place would reassure that this does not break in the future

@petebacondarwin
Copy link
Contributor

Quick work @hannahhoward - nicely done and nice PR. Thanks

@hannahhoward
Copy link
Contributor Author

added second test per request. and removed the accidental leftover console.log. I'm not sure why the build is failing on Firefox 31.0 on Linux

@@ -2156,7 +2156,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
beforeTemplateCompileNode = $compileNode[0],
origAsyncDirective = directives.shift(),
// The fact that we have to copy and patch the directive seems wrong!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment doesn't reflect the code below it anymore.

@hannahhoward
Copy link
Contributor Author

removed the outdated comment

@lgalfaso
Copy link
Contributor

lgalfaso commented Feb 2, 2015

Looks very good. Just one more thing @hannahhoward can you please squash everything into one commit? Thanks!

@petebacondarwin
Copy link
Contributor

Actually @hannahhoward don't worry, we can squash when we merge.

@petebacondarwin petebacondarwin self-assigned this Feb 2, 2015
petebacondarwin pushed a commit that referenced this pull request Feb 2, 2015
Previously, if a directive definition object was defined with methods like `compile`
provided on the prototype rather than the instance, the Angular compiler failed
to use these methods when the directive had a `templateURL`. This change ensures
that these prototypical methods are not lost.

This enables developers to define their directives using "classes" such as
in CoffeeScript or ES6.

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

Successfully merging this pull request may close these issues.

6 participants