Opened 8 years ago
Closed 8 years ago
#15437 closed defect (fixed)
remove AMD stub code from deviceTheme.js
Reported by: | bill | Owned by: | Eric Durocher |
---|---|---|---|
Priority: | undecided | Milestone: | 1.8 |
Component: | DojoX Mobile | Version: | 1.7.2 |
Keywords: | Cc: | cjolif, Colin Snover | |
Blocked By: | Blocking: |
Description
Similar to the old scrollable.js, deviceTheme.js tries to create a stub define() if it isn't there.
var _define; if(typeof define === "undefined"){ // assumes dojo.js is not loaded define = _define = function(module, deps, def){ ((arguments.length === 2) ? arguments[1] : arguments[2])(); }; }
and also at the end of the file.
This should be removed (as we discussed in the dojo meeting) to conform to the rest of the code in dojo's SVN.
Attachments (2)
Change History (7)
comment:1 Changed 8 years ago by
Cc: | cjolif added |
---|---|
Milestone: | tbd → 1.8 |
comment:2 Changed 8 years ago by
Note that the use case here is not quite the same as scrollable.js. The code in scrollable.js was there so that the module could be used *without* dojo (and this use case was abandoned and the code removed), whereas here, deviceTheme.js can be loaded *just before* dojo.js. This use case is documented in the module's doc and is used extensively in the test cases, so we cannot just remove this code like in scrollable.js.
If this confuses the doc parser I see 2 solutions:
1: use the same trick as in [28628], that is add doc comments to help the doc parser (patch 1).
2: use a conditional expression to call the define-callback function directly if define is not yet defined (patch 2).
Solution 2 is slightly cleaner because it avoids the redefinition of the global define, but solution 1 is safest since it does not change the code at all, which might be a better option given the proximity to the beta...
Changed 8 years ago by
Attachment: | 15437.1.patch added |
---|
Solution 1: add doc comments - Eric Durocher (IBM, CCLA)
Changed 8 years ago by
Attachment: | 15437.2.patch added |
---|
Solution 2: use conditional expression - Eric Durocher (IBM, CCLA)
comment:3 Changed 8 years ago by
Cc: | Colin Snover added |
---|
OK, well the current code does "confuse" the doc parser, although currently deviceTheme.js is excluded from the doc parse, see config.js. Either of your patches makes the file parse.
I don't like how the file can be loaded in two separate ways (either loaded via require(), or via <script>), but if you want to keep supporting both of those options, then either patch seems fine to me. I agree that solution 2 is cleaner.
comment:4 Changed 8 years ago by
bill, we know this is not clean and longer term we plan revisit this. The ability to use <script> to load this module is here for now to limit the effects of CSS files being loaded to late in the cycle and the other way for compatibility reasons.
Marking for 1.8 since this likely will confuse the AMD doc parser.