Fix template overrides - decorate script directive to prevent re-overriding of templates
#113
Conversation
not really needed, but is consistent with how done elsewhere, and allows override
by default `<script>` will install to template cache whenever it is processed as a directive. this decorates it to be no-op if there is already something in the cache with that name, allowing programmatic overrides of templates configured using `<script id="...">` notation.
|
It works fine for me 👍 |
| @@ -79,7 +82,7 @@ export function specEditorDirective($rootScope, $templateCache, $injector, $sani | |||
| model: '=' | |||
| }, | |||
| controller: ['$scope', '$element', controller], | |||
| template: template, | |||
| templateUrl: TEMPLATE_URL, | |||
There was a problem hiding this comment.
While you do this, could you change it to match what the other directives do?
So it can be overridden directly from the attribute template-url on the directive itself.
There was a problem hiding this comment.
good spot, sorry i missed this!
| let base = $delegate[0]; | ||
| return [ Object.assign({}, base, { compile: function(el, attr) { | ||
| let match = $templateCache.get(attr.id); | ||
| if (!(match === null || typeof match === 'undefined')) { |
There was a problem hiding this comment.
I would be reluctant to change the default behaviour for the entire application without a possibility to go back. I think it would be wise to add a override (or a better name) parameter to revert to the default behaviour if we need/want to
There was a problem hiding this comment.
i wondered about this too. we have unique names for all our things and i don't see any way someone would have registered a template that isn't meant to override our things, so didn't think we needed an "opt-out".
however there is a risk with 3rd party libraries that templates might have non-unique names and this could cause an issue there. doesn't seem to be a problem but if we're concerned what i'd suggest is that we make this behaviour "opt-in", checking an extra attribute e.g. defer-to-preexisting-id, which we set everywhere in our code that we define a <script id="..." ... tag.
not sure it's needed but will add if you think so @tbouron .
There was a problem hiding this comment.
@ahgittin Aye, that was the use-case I had in mind. I don't think it's strictly speaking required but better safe than sorry => I think it would be better to do this change.
Although, there is another solution that is even simpler: don't decorate the script directive and update the way we register template to do that in a .config block (like the main template) rather than using <script> tags
There was a problem hiding this comment.
okay, done @tbouron , see last commit
this allows scripts in templates to "opt-in" to being overridable; our custom decorator has no effect unless this attribute is set, so we guarantee not to change any behaviour in 3rd party libraries
ahgittin
force-pushed
the
fix-template-overrides
branch
from
664f4ea
to
116c50e
Compare
by default
<script>will install to template cache whenever it is processed as a directive.this decorates it to be no-op if there is already something in the cache with that name,
allowing programmatic overrides of templates configured using
<script id="...">notation.