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)

15437.1.patch (550 bytes) - added by Eric Durocher 8 years ago.
Solution 1: add doc comments - Eric Durocher (IBM, CCLA)
15437.2.patch (797 bytes) - added by Eric Durocher 8 years ago.
Solution 2: use conditional expression - Eric Durocher (IBM, CCLA)

Download all attachments as: .zip

Change History (7)

comment:1 Changed 8 years ago by bill

Cc: cjolif added
Milestone: tbd1.8

Marking for 1.8 since this likely will confuse the AMD doc parser.

comment:2 Changed 8 years ago by Eric Durocher

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 Eric Durocher

Attachment: 15437.1.patch added

Solution 1: add doc comments - Eric Durocher (IBM, CCLA)

Changed 8 years ago by Eric Durocher

Attachment: 15437.2.patch added

Solution 2: use conditional expression - Eric Durocher (IBM, CCLA)

comment:3 Changed 8 years ago by bill

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 cjolif

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.

comment:5 Changed 8 years ago by cjolif

Resolution: fixed
Status: newclosed

In [28702]:

fixes #15437. Thanks Eric Durocher (IBM, CCLA).

Note: See TracTickets for help on using tickets.