Opened 14 years ago
Closed 14 years ago
#2251 closed defect (wontfix)
do not run require commands if it is commented out by // when debugAtAllCosts=true
Reported by: | liucougar | Owned by: | liucougar |
---|---|---|---|
Priority: | high | Milestone: | 0.9 |
Component: | General | Version: | 0.4.1 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description
the patch prevents executing of source code like this:
// dojo.require("dojo.widget.Editor2Plugin.ContextMenu");
(code commented out by /* */ will still be executed though, but at least this patch has fixed part of the problem before removeComments is fixed to strip comments reliably)
Attachments (2)
Change History (6)
comment:1 Changed 14 years ago by
Changed 14 years ago by
Attachment: | 2251.patch added |
---|
Changed 14 years ago by
Attachment: | 2251.2.patch added |
---|
comment:2 Changed 14 years ago by
2251.2.patch does the trick another way:
it will only match dojo.require beginning with spaces or nothing
which do you think is better?
comment:3 Changed 14 years ago by
I think 2251.2.patch will not work if the user has more than one dojo.require() call on a line. While I don't think that is too big of an issue right now, I hope we can later allow for "minifying" all Dojo modules as part of a build process, and in that minify case (where excessive white space/line and line returns are removed), then it might be an issue.
I favor changing removeComments to only do single-line comment removal, but keep all the old code and comment so we know it is not ready for general use. Then, call removeComments before calling the getRequiresAndProvides and getDelayRequiresAndProvides functions (the functions that look for the dojo.require/dojo.requireIf dependencies). Even though removeComments won't be perfect (it might have issues with in strings and regexps), it should be fine for using it when looking for dojo.require/requireIf dependencies.
comment:4 Changed 14 years ago by
Resolution: | → wontfix |
---|---|
Status: | new → closed |
debugAtAllCosts is gone, so this is no longer an issue
updated the regex according to peller's advice; a bit more comments