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

fix($compile): remove comment nodes from templates before asserting single root node #9215

Closed
wants to merge 1 commit into from

Conversation

caitp
Copy link
Contributor

@caitp caitp commented Sep 22, 2014

The compiler will no longer throw if a directive template contains comment
nodes in addition to a single root node.

The comment nodes will be removed before processing.

Closes #9212

@lgalfaso
Copy link
Contributor

So if the template is a single comment that makes reference to a directive, then it would stop working?

@caitp
Copy link
Contributor Author

caitp commented Sep 22, 2014

yeah pretty much --- in practice nobody actually does that though, Igor has said in the past that supporting comment directives at all was probably a mistake

@caitp
Copy link
Contributor Author

caitp commented Sep 22, 2014

If it makes you feel better, we could avoid changing the object unless there is more than one node

…ingle root node

The compiler will no longer throw if a directive template contains comment nodes in addition to a single root node.

The comment nodes will be removed before processing.

Closes angular#9212
@caitp
Copy link
Contributor Author

caitp commented Sep 22, 2014

now avoids the breaking change =)

@lgalfaso
Copy link
Contributor

Igor has said in the past that supporting comment directives at all was probably a mistake

Agree

LGTM

@btford
Copy link
Contributor

btford commented Sep 22, 2014

LGTM

@@ -1951,7 +1951,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
if (jqLiteIsTextNode(content)) {
$template = [];
} else {
$template = jqLite(wrapTemplate(templateNamespace, trim(content)));
$template = removeComments(jqLite(wrapTemplate(templateNamespace, trim(content))));
Copy link
Contributor

Choose a reason for hiding this comment

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

how many more function calls can we make in this line of code? :)

@IgorMinar
Copy link
Contributor

otherwise this looks good to me.

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