[FIX] SapUiDefine call should not fail when there's no factory functi…

…on (#491)

Other existing implementations, like the one in ui5-migration tooling, had already been fixed in similar ways.

lib/lbt/calls/SapUiDefine.js

@@ -2,25 +2,37 @@
 
 const {Syntax} = require("esprima");
 const ModuleName = require("../utils/ModuleName");
-const {isString} = require("../utils/ASTUtils");
+const {isString, isBoolean} = require("../utils/ASTUtils");
 
 class SapUiDefineCall {
 	constructor(node, moduleName) {
 		this.node = node;
 		this.name = moduleName;
 		this.dependencyArray = null;
 		this.factory = null;
+		this.exportAsGlobal = false;
 
 		const args = node.arguments;
+		if ( args == null ) {
+			return;
+		}
+
+		// Note: the following code assumes that no variables or expressions are used
+		// for the arguments of the sap.ui.define call. The analysis could be made more
+		// sophisticated and could try to skip unhandled parameter types, based on the
+		// AST type of follow-up arguments.
+		// But on the other hand, an incomplete analysis of the define call is useless in
+		// many cases, so it might not be worth the effort.
+
 		let i = 0;
 		let params;
 
-		if ( args[i].type === Syntax.Literal ) {
+		if ( i < args.length && isString(args[i]) ) {
 			// assert(String)
 			this.name = args[i++].value;
 		}
 
-		if ( args[i].type === Syntax.ArrayExpression ) {
+		if ( i < args.length && args[i].type === Syntax.ArrayExpression ) {
 			this.dependencyArray = args[i++];
 			this.dependencies = this.dependencyArray.elements.map( (elem) => {
 				if ( !isString(elem) ) {
@@ -31,7 +43,7 @@ class SapUiDefineCall {
 			this.dependencyInsertionIdx = this.dependencyArray.elements.length;
 		}
 
-		if ( args[i].type === Syntax.FunctionExpression ) {
+		if ( i < args.length && args[i].type === Syntax.FunctionExpression ) {
 			this.factory = args[i++];
 			params = this.factory.params;
 			this.paramNames = params.map( (param) => {
@@ -45,6 +57,10 @@ class SapUiDefineCall {
 			}
 		}
 
+		if ( i < args.length && isBoolean(args[i]) ) {
+			this.exportAsGlobal = args[i].value;
+		}
+
 		// console.log("declared dependencies: " + this.dependencies);
 	}
 

lib/lbt/utils/ASTUtils.js

@@ -19,6 +19,13 @@ function isString(node, literal) {
 	return literal == null ? true : node.value === literal;
 }
 
+function isBoolean(node, literal) {
+	if ( node == null || node.type !== Syntax.Literal || typeof node.value !== "boolean" ) {
+		return false;
+	}
+	return literal == null ? true : node.value === literal;
+}
+
 function isMethodCall(node, methodPath) {
 	if ( node.type !== Syntax.CallExpression ) {
 		return false;
@@ -106,6 +113,7 @@ function getStringArray(array, skipNonStringLiterals) {
 
 module.exports = {
 	isString,
+	isBoolean,
 	isMethodCall,
 	isNamedObject,
 	isIdentifier,

test/lib/lbt/calls/SapUiDefine.js

@@ -9,6 +9,12 @@ function parse(code) {
 	return ast.body[0].expression;
 }
 
+test("Empty Define", (t) => {
+	const ast = parse("sap.ui.define();");
+	const call = new SapUiDefineCall(ast, "FileSystemName");
+	t.true(call != null, "call could be parsed");
+});
+
 test("Named Define", (t) => {
 	const ast = parse("sap.ui.define('HardcodedName', [], function() {});");
 	const call = new SapUiDefineCall(ast, "FileSystemName");
@@ -42,6 +48,13 @@ test("Factory", (t) => {
 	t.is(call.factory.type, Syntax.FunctionExpression);
 });
 
+test("No Factory", (t) => {
+	const ast = parse("sap.ui.define(['a', 'b']);");
+	const call = new SapUiDefineCall(ast, "FileSystemName");
+	t.deepEqual(call.dependencies, ["a.js", "b.js"]);
+	t.is(call.factory, null);
+});
+
 test("Find Import Name (successful)", (t) => {
 	const ast = parse("sap.ui.define(['wanted'], function(johndoe) {});");
 	const call = new SapUiDefineCall(ast, "FileSystemName");
@@ -59,3 +72,22 @@ test("Find Import Name (no dependencies)", (t) => {
 	const call = new SapUiDefineCall(ast, "FileSystemName");
 	t.is(call.findImportName("wanted.js"), null);
 });
+
+test("Export as Global: omitted", (t) => {
+	const ast = parse("sap.ui.define(['wanted'], function(johndoe) {});");
+	const call = new SapUiDefineCall(ast, "FileSystemName");
+	t.is(call.exportAsGlobal, false);
+});
+
+test("Export as Global: false", (t) => {
+	const ast = parse("sap.ui.define(['wanted'], function(johndoe) {}, false);");
+	const call = new SapUiDefineCall(ast, "FileSystemName");
+	t.is(call.exportAsGlobal, false);
+});
+
+test("Export as Global: true", (t) => {
+	const ast = parse("sap.ui.define(['wanted'], function(johndoe) {}, true);");
+	const call = new SapUiDefineCall(ast, "FileSystemName");
+	t.is(call.exportAsGlobal, true);
+});
+

test/lib/lbt/utils/ASTUtils.js

@@ -14,6 +14,24 @@ test("isString", (t) => {
 	t.false(ASTUtils.isString(literal, "myOtherValue47"), "is a literal but its value does not match");
 });
 
+test("isBoolean", (t) => {
+	t.false(ASTUtils.isString(null));
+
+	const trueLiteral = esprima.parse("true").body[0].expression;
+	const falseLiteral = esprima.parse("false").body[0].expression;
+	const stringLiteral = esprima.parse("'some string'").body[0].expression;
+	const call = esprima.parse("setTimeout()").body[0];
+
+	t.true(ASTUtils.isBoolean(trueLiteral), "is a boolean literal");
+	t.true(ASTUtils.isBoolean(falseLiteral), "is a boolean literal");
+	t.false(ASTUtils.isBoolean(stringLiteral), "is not a boolean literal");
+	t.false(ASTUtils.isBoolean(call), "is not a boolean literal");
+	t.true(ASTUtils.isBoolean(trueLiteral, true), "is a literal and its value matches");
+	t.false(ASTUtils.isBoolean(trueLiteral, false), "is a literal and value does not matches");
+	t.true(ASTUtils.isBoolean(falseLiteral, false), "is a literal and its value matches");
+	t.false(ASTUtils.isBoolean(falseLiteral, true), "is a literal and value does not matches");
+});
+
 test("isIdentifier", (t) => {
 	const literal = esprima.parse("'testValue47'").body[0].expression;