Opened 6 years ago

Closed 6 years ago

#16368 closed enhancement (fixed)

[patch][ccla] add factory scan to build

Reported by: ben hockey Owned by: Rawld Gill
Priority: low Milestone: 1.9
Component: BuildSystem Version: 1.8.1
Keywords: Cc:
Blocked By: Blocking:

Description

in the code (http://bugs.dojotoolkit.org/browser/dojo/util/trunk/build/transforms/depsScan.js?rev=28477#L36) there is a section for factory scanning but currently it's disabled.

is there a compelling reason not to enable it?

it could be guarded by a flag (eg bc.scanFactory) so as to be opt-in and not surprise anyone who may be relying on factory scans being disabled.

Attachments (1)

scan.diff (1.6 KB) - added by Bryan Forbes 6 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 6 years ago by ben hockey

Summary: add factory scan to build[patch][ccla] add factory scan to build
  • util/build/transforms/depsScan.js

     
    3333                                        args = 0,
    3434                                        defaultDeps = ["require", "exports", "module"];
    3535
    36                                 // TODO: add the factory scan?
    37                                 if(0){
     36                                if(bc.scanFactory){
    3837                                        if(arity==1){
    3938                                                dependencies = [];
    4039                                                mid.toString()

comment:2 Changed 6 years ago by Rawld Gill

Milestone: tbd1.8.4
Status: newassigned

comment:3 Changed 6 years ago by Rawld Gill

Type: defectenhancement

comment:4 Changed 6 years ago by Rawld Gill

Priority: undecidedlow

comment:5 Changed 6 years ago by Bryan Forbes

The solution proposed is a bit naive. The factory scanning portion of the loader gets built out by default and factory scanning doesn't work in all browsers (most notably older Opera Mobile browsers). This means that when a module using factory scanning is hit, the module it needs may or may not be defined. This means that as part of the build process for these modules, something needs to signal to the loader that the modules are required.

I had two thoughts on how to solve this problem. The first is to wrap the define(function(){}) in a require([], function(){}) that has the modules needed by the define():

define(function(require){
    var dep1 = require('dep1'),
        dep2 = require('dep2');
});

Gets changed to:

require(['dep1', 'dep2'], function(){
    define(function(require){
        var dep1 = require('dep1'),
            dep2 = require('dep2');
    });
});

The second idea I had involves rewriting the define() in the build:

define(function(require){
    var dep1 = require('dep1'),
        dep2 = require('dep2');
});

Gets rewritten to:

define(['require', 'exports', 'module', 'dep1', 'dep2'], function(require){
    var dep1 = require('dep1'),
        dep2 = require('dep2');
});

Admittedly, the first suggestion seems less invasive and less prone to errors given that it's not rewriting module contents. If you can think of a better way to do it, that would be great as well :).

comment:6 Changed 6 years ago by Bryan Forbes

After thinking about this and discussing with neonstalwart, the second solution is probably the only one that would work. In the first solution, relative IDs would need to be transformed to absolute IDs which means mapping and insertAbsMid: 0 would not work with it. Darn reality getting in the way of easy solutions!

comment:7 Changed 6 years ago by Colin Snover

Is there a reason why this is assigned M=1.8.4?

comment:8 Changed 6 years ago by Colin Snover

Milestone: 1.8.41.9

Moving enhancements to next major release.

comment:9 Changed 6 years ago by Bryan Forbes

I have attached a patch to enable the factory scan and also implement the second solution mentioned above. If there's a better way, I'm not opposed to implementing that, either.

Changed 6 years ago by Bryan Forbes

Attachment: scan.diff added

comment:10 Changed 6 years ago by Rawld Gill

In [31169]:

added opt-in for factory scan during dependency scanning; refs #16368; !strict

comment:11 Changed 6 years ago by Rawld Gill

@BryanForbes?, @neonstalwart: take a look at [31169] (above) and let me know if you have any suggestions. Thanks for the help on this.

comment:12 Changed 6 years ago by Rawld Gill

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.