Opened 11 years ago

Closed 6 years ago

#5456 closed enhancement (patchwelcome)

HTML comments in templates are not removed when template strings are interned.

Reported by: ptwobrussell Owned by: Rawld Gill
Priority: low Milestone: 1.9
Component: BuildTools Version: 1.0
Keywords: needsreview Cc: ptwobrussell@…
Blocked By: Blocking:

Description (last modified by James Burke)

I noticed that HTML comments aren't removed when template strings are interned. HTML comments can be handy when you have a complex layout with layout dijits and you are trying to keep track of what is what...or if you have to put a copyright blurb in the template during development per a company policy. Seems like stripping these comments for a build would make sense.

Not sure who would be the best person to assign this to, so I thought I'd start somewhere.

Change History (12)

comment:1 Changed 11 years ago by Adam Peller

Owner: changed from dante to James Burke

Is the bug that HTML comments aren't ever being removed, or only in a particular case?

comment:2 Changed 11 years ago by ptwobrussell

The HTML comments are never being removed and they appear in my final compressed .js file that's been through shrink safe and the whole 9 yards.

comment:3 Changed 11 years ago by James Burke

Milestone: 1.1

I thought there was an existing bug for this issue (it is a build system issue), but I cannot seem to find it. I'll see about getting into 1.1, but no guarantees yet.

comment:4 Changed 11 years ago by dylan

I would argue that ideally this would be a flag also, as there are cases where inserting comment nodes might be desirable.

comment:5 Changed 11 years ago by James Burke

dylan: so you want an option to keep comments in the inlined string (the value set for templateString)? Do you see the comment nodes useful just in development or after a build that is used for "production" use?

Or is more of an issue where removing the comment nodes affects the number of children in a node, and if some code is assuming certain child positions, the code could break? If that is the case, then I am more reticent to do this fix, or perhaps getting the comment stripping behavior is an opt-in option instead of a default that you have to opt out.

comment:6 Changed 11 years ago by dylan

Two possible production use cases. One is like you said, number of nodes. The other is the use case of using a comment node as a place to store data until needed.

comment:7 Changed 11 years ago by Adam Peller

For the first case, I think it's possible, but we should discourage references by index. dojoAttachPoint makes for much less fragile code in general. By that argument, I'd try to make the case that stripping comments should be an opt-out thing, but I don't feel too strongly about it as long as it's an option.

At the risk of over-engineering, if contents in the comments really mattered, we could have a convention for a special comment syntax, e.g. !--*, that is always included and calls attention to the content?

comment:8 Changed 11 years ago by James Burke

Milestone: 1.11.2

comment:9 Changed 11 years ago by James Burke

Description: modified (diff)
Milestone: 1.2future

comment:10 Changed 7 years ago by ben hockey

Keywords: needsreview added

comment:11 Changed 7 years ago by bill

Owner: changed from James Burke to Rawld Gill
Status: newassigned

Personally I would just mark as wontfix, but leaving up to you.

Last edited 7 years ago by bill (previous) (diff)

comment:12 Changed 6 years ago by dylan

Milestone: future1.9
Resolution: patchwelcome
Status: assignedclosed

Given the age of this ticket, a patch would be welcome, otherwise this will never be fixed.

Note: See TracTickets for help on using tickets.