Skip to content

Commit 90ff54a

Browse files
jrburkejrburke
committed
Fixes requirejs/requirejs#1133, UMD wrapper detection was too aggressive
Instead, pass scope of function expression identifiers, and only skip parsing if define uses one of those identifiers.
1 parent 295befe commit 90ff54a

File tree

14 files changed

+269
-29
lines changed

14 files changed

+269
-29
lines changed

‎.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ build/tests/lib/nestedHas/main-builtNeedD.js
5454
build/tests/lib/nestedHas/main-builtNested.js
5555
build/tests/lib/noexports/main-built.js
5656
build/tests/lib/nonStrict/main-built.js
57+
build/tests/lib/nonUmdIdentifiers/main-built.js
5758
build/tests/lib/onBuildAllDir/js-built
5859
build/tests/lib/onBuildRead/main-built.js
5960
build/tests/lib/onBuildWrite/main-built.js

‎build/jslib/parse.js

Lines changed: 48 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,11 @@ define(['./esprimaAdapter', 'lang'], function (esprima, lang) {
2424

2525
//This string is saved off because JSLint complains
2626
//about obj.arguments use, as 'reserved word'
27-
var argPropName = 'arguments';
27+
var argPropName = 'arguments',
28+
//Default object to use for "scope" checking for UMD identifiers.
29+
emptyScope = {},
30+
mixin = lang.mixin,
31+
hasProp = lang.hasProp;
2832

2933
//From an esprima example for traversing its ast.
3034
function traverse(object, visitor) {
@@ -126,7 +130,7 @@ define(['./esprimaAdapter', 'lang'], function (esprima, lang) {
126130
needsDefine = true,
127131
astRoot = esprima.parse(fileContents);
128132

129-
parse.recurse(astRoot, function (callName, config, name, deps, node, factoryIdentifier) {
133+
parse.recurse(astRoot, function (callName, config, name, deps, node, factoryIdentifier, fnExpScope) {
130134
if (!deps) {
131135
deps = [];
132136
}
@@ -146,7 +150,7 @@ define(['./esprimaAdapter', 'lang'], function (esprima, lang) {
146150
});
147151
}
148152

149-
if (factoryIdentifier) {
153+
if (callName === 'define' && factoryIdentifier && hasProp(fnExpScope, factoryIdentifier)) {
150154
return factoryIdentifier;
151155
}
152156

@@ -199,14 +203,18 @@ define(['./esprimaAdapter', 'lang'], function (esprima, lang) {
199203
* @param {Function} onMatch function to call on a parse match.
200204
* @param {Object} [options] This is normally the build config options if
201205
* it is passed.
206+
* @param {Object} [fnExpScope] holds list of function expresssion
207+
* argument identifiers, set up internally, not passed in
202208
*/
203-
parse.recurse = function (object, onMatch, options) {
209+
parse.recurse = function (object, onMatch, options, fnExpScope) {
204210
//Like traverse, but skips if branches that would not be processed
205211
//after has application that results in tests of true or false boolean
206212
//literal values.
207-
var key, child, result,
213+
var key, child, result, i, params, param,
208214
hasHas = options && options.has;
209215

216+
fnExpScope = fnExpScope || emptyScope;
217+
210218
if (!object) {
211219
return;
212220
}
@@ -217,24 +225,44 @@ define(['./esprimaAdapter', 'lang'], function (esprima, lang) {
217225
object.test.type === 'Literal') {
218226
if (object.test.value) {
219227
//Take the if branch
220-
this.recurse(object.consequent, onMatch, options);
228+
this.recurse(object.consequent, onMatch, options, fnExpScope);
221229
} else {
222230
//Take the else branch
223-
this.recurse(object.alternate, onMatch, options);
231+
this.recurse(object.alternate, onMatch, options, fnExpScope);
224232
}
225233
} else {
226-
result = this.parseNode(object, onMatch);
234+
result = this.parseNode(object, onMatch, fnExpScope);
227235
if (result === false) {
228236
return;
229237
} else if (typeof result === 'string') {
230238
return result;
231239
}
232240

241+
//Build up a "scope" object that informs nested recurse calls if
242+
//the define call references an identifier that is likely a UMD
243+
//wrapped function expresion argument.
244+
if (object.type === 'ExpressionStatement' && object.expression &&
245+
object.expression.type === 'CallExpression' && object.expression.callee &&
246+
object.expression.callee.type === 'FunctionExpression') {
247+
object = object.expression.callee;
248+
249+
if (object.params && object.params.length) {
250+
params = object.params;
251+
fnExpScope = mixin({}, fnExpScope, true);
252+
for (i = 0; i < params.length; i++) {
253+
param = params[i];
254+
if (param.type === 'Identifier') {
255+
fnExpScope[param.name] = true;
256+
}
257+
}
258+
}
259+
}
260+
233261
for (key in object) {
234262
if (object.hasOwnProperty(key)) {
235263
child = object[key];
236264
if (typeof child === 'object' && child !== null) {
237-
result = this.recurse(child, onMatch, options);
265+
result = this.recurse(child, onMatch, options, fnExpScope);
238266
if (typeof result === 'string') {
239267
break;
240268
}
@@ -246,23 +274,10 @@ define(['./esprimaAdapter', 'lang'], function (esprima, lang) {
246274
//passed in as a function expression, indicating a UMD-type of
247275
//wrapping.
248276
if (typeof result === 'string') {
249-
if (object.type === 'ExpressionStatement' && object.expression &&
250-
object.expression.type === 'CallExpression' && object.expression.callee &&
251-
object.expression.callee.type === 'FunctionExpression') {
252-
object = object.expression.callee;
253-
254-
if (object.params && object.params.length) {
255-
if (object.params.some(function(param) {
256-
//Found an identifier match, so stop parsing from this
257-
//level down.
258-
return param.type === 'Identifier' &&
259-
param.name === result;
260-
})) {
261-
//Just a plain return, parsing can continue past this
262-
//point.
263-
return;
264-
}
265-
}
277+
if (hasProp(fnExpScope, result)) {
278+
//Just a plain return, parsing can continue past this
279+
//point.
280+
return;
266281
}
267282

268283
return result;
@@ -740,11 +755,14 @@ define(['./esprimaAdapter', 'lang'], function (esprima, lang) {
740755
* @param {Function} onMatch a function to call when a match is found.
741756
* It is passed the match name, and the config, name, deps possible args.
742757
* The config, name and deps args are not normalized.
758+
* @param {Object} fnExpScope an object whose keys are all function
759+
* expression identifiers that should be in scope. Useful for UMD wrapper
760+
* detection to avoid parsing more into the wrapped UMD code.
743761
*
744762
* @returns {String} a JS source string with the valid require/define call.
745763
* Otherwise null.
746764
*/
747-
parse.parseNode = function (node, onMatch) {
765+
parse.parseNode = function (node, onMatch, fnExpScope) {
748766
var name, deps, cjsDeps, arg, factory, exp, refsDefine, bodyNode,
749767
args = node && node[argPropName],
750768
callName = parse.hasRequire(node);
@@ -818,7 +836,8 @@ define(['./esprimaAdapter', 'lang'], function (esprima, lang) {
818836
}
819837

820838
return onMatch("define", null, name, deps, node,
821-
(factory && factory.type === 'Identifier' ? factory.name : undefined));
839+
(factory && factory.type === 'Identifier' ? factory.name : undefined),
840+
fnExpScope);
822841
} else if (node.type === 'CallExpression' && node.callee &&
823842
node.callee.type === 'FunctionExpression' &&
824843
node.callee.body && node.callee.body.body &&
@@ -846,7 +865,7 @@ define(['./esprimaAdapter', 'lang'], function (esprima, lang) {
846865

847866
if (refsDefine) {
848867
return onMatch("define", null, null, null, exp.expression,
849-
exp.expression.arguments[0].name);
868+
exp.expression.arguments[0].name, fnExpScope);
850869
}
851870
}
852871
}

‎build/tests/builds.js

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2351,6 +2351,26 @@ define(['build', 'env!env/file', 'env', 'lang'], function (build, file, env, lan
23512351
);
23522352
doh.run();
23532353

2354+
//Keep parsing if a define uses an identifier, but is not part of
2355+
//a function wrapper for UMD
2356+
//https://github.com/jrburke/requirejs/issues/1133
2357+
doh.register("nonUmdIdentifiers",
2358+
[
2359+
function nonUmdIdentifiers(t) {
2360+
file.deleteFile("lib/nonUmdIdentifiers/main-built.js");
2361+
2362+
build(["lib/nonUmdIdentifiers/build.js"]);
2363+
2364+
t.is(nol(c("lib/nonUmdIdentifiers/expected.js")),
2365+
nol(c("lib/nonUmdIdentifiers/main-built.js")));
2366+
2367+
require._buildReset();
2368+
}
2369+
2370+
]
2371+
);
2372+
doh.run();
2373+
23542374
//out function should get source map if available.
23552375
//https://github.com/jrburke/r.js/issues/590
23562376
doh.register("sourcemapOutFunction",

‎build/tests/lib/nonUmdIdentifiers/angular.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
define({ name: 'angular' });

‎build/tests/lib/nonUmdIdentifiers/app.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
define({ name: 'app' });

‎build/tests/lib/nonUmdIdentifiers/build.js

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
baseUrl: '.',
3+
name: 'main',
4+
out: 'main-built.js',
5+
findNestedDependencies: true,
6+
optimize: 'none'
7+
}

‎build/tests/lib/nonUmdIdentifiers/debug.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
define({ name: 'debug' });

‎build/tests/lib/nonUmdIdentifiers/expected.js

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
define('jquery',{ name: 'jquery' });
2+
3+
define('gsn',{ name: 'gsn' });
4+
5+
define('gsn-extensions',{ name: 'gsn-extensions' });
6+
7+
define('debug',{ name: 'debug' });
8+
9+
define('jqueryui',{ name: 'jqueryiui' });
10+
11+
define('app',{ name: 'app' });
12+
13+
define('angular',{ name: 'angular' });
14+
15+
define('states',{ name: 'states' });
16+
17+
(function (global) {
18+
var loadEvents = (function loadTimerObj() {
19+
}()).add('main-init');
20+
21+
define('loadEvents', [], loadEvents);
22+
23+
if (global.__gsnConfig) {
24+
define('gsnConfig', [], global.__gsnConfig);
25+
} else {
26+
define('gsnConfig', [], { empty: true });
27+
}
28+
29+
if (global.__requireConfig) {
30+
define('requireConfig', [], global.__requireConfig);
31+
} else {
32+
define('requireConfig', [], { empty: true });
33+
}
34+
35+
if (global.__gaq) {
36+
define('gaq', [], global._gaq);
37+
} else {
38+
define('gaq', [], []);
39+
}
40+
41+
if (global.__index) {
42+
define('index', [], global.__index);
43+
} else {
44+
define('index', [], {});
45+
}
46+
47+
define('modernizr', [], global.Modernizr);
48+
49+
require(['requireConfig', 'gsnConfig'], function (requireConfig, gsnConfig) {
50+
51+
52+
var paymentsShim = {}, legacyShim = {}, debugFilter;
53+
54+
gsnConfig.dropInHeader = global.__dropInHeader;
55+
56+
define('legacyShim', [], legacyShim);
57+
if (gsnConfig.useLegacyShim) {
58+
global.redesignShim = legacyShim;
59+
}
60+
61+
62+
define('paymentsShim', [], paymentsShim);
63+
if (gsnConfig.usePaymentsShim) {
64+
global.paymentsShim = {};
65+
}
66+
67+
var
68+
jqKeys = { jquery: 'jquery', ui: 'jqueryui' },
69+
jQueryAlreadyDefined = global.jQuery,
70+
jQueryUIAlreadyDefined = global.jQuery && global.jQuery.ui;
71+
if (jQueryAlreadyDefined) {
72+
define(jqKeys.jquery, [], function() { return global.jQuery; });
73+
}
74+
if (jQueryUIAlreadyDefined) {
75+
define(jqKeys.ui, [], function () { return {}; });
76+
}
77+
78+
require(['jquery'], function ($) {
79+
});
80+
81+
require([
82+
'jquery',
83+
'gsn',
84+
'gsn-extensions',
85+
'debug',
86+
87+
'jqueryui'
88+
], function ($, gsn, gsnExtensions, debugLogger) {
89+
function start() {}
90+
91+
require([
92+
'app',
93+
'angular',
94+
'modernizr',
95+
96+
'states'
97+
], start);
98+
});
99+
});
100+
})(this);
101+

‎build/tests/lib/nonUmdIdentifiers/gsn-extensions.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
define({ name: 'gsn-extensions' });

‎build/tests/lib/nonUmdIdentifiers/gsn.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
define({ name: 'gsn' });

0 commit comments

Comments
 (0)