Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve naming conflict when the configured name is already used in code #250

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/dependencies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ export interface Extender {
*
* @param {SapUiDefineCall} defineCall the module's define call
* @param {object} config is passed along from finder and replacer
* @param {string} [name] is the unique variable name when naming conflict is found
* @returns {boolean} whether or not the defineCall was modified
*/
extend(defineCall: SapUiDefineCall, config: {}): boolean;
extend(defineCall: SapUiDefineCall, config: {}, name?: string): boolean;
}
83 changes: 80 additions & 3 deletions src/tasks/addMissingDependencies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ function mapToFound(oPath: NodePath, oFound: FoundReplacement): FoundCall {
replacerName: oFound.oModuleInner.replacer,
extenderName: oFound.oModuleInner.extender,
newModulePath: oFound.oModuleInner.newModulePath,
newVariableName: oFound.uniqueVariableName,
},
};
}
Expand Down Expand Up @@ -191,7 +192,11 @@ function isFoundInConfig(
oNodePath: NodePath,
oModuleTree: {
[index: string]: {
[index: string]: {finder: string; newModulePath: string};
[index: string]: {
finder: string;
newModulePath: string;
newVariableName?: string;
};
};
},
finder: {[name: string]: Finder},
Expand All @@ -204,6 +209,66 @@ function isFoundInConfig(
for (const sModuleInner in oModule) {
if (oModule.hasOwnProperty(sModuleInner)) {
const oModuleInner = oModule[sModuleInner];

// 1. newVariable is unique
// -> OK, we just add a new import and use the variable name
// 2. newVariableName is already in use - same import path
// -> OK, we just use the existing import and use the variable name
// 3. newVariableName is already in use - different import path
// -> NotOK, we need to make the newVariableName unique and add the import
let candidateName = oModuleInner.newVariableName;
if (
oModuleInner.newVariableName &&
oModuleInner.newModulePath
) {
let existingModulePath =
defineCall.getImportByParamName(
oModuleInner.newVariableName
);

if (
existingModulePath &&
existingModulePath !== oModuleInner.newModulePath
) {
const pathParts =
oModuleInner.newModulePath.split("/");
if (pathParts.length >= 2) {
// concatenate name suffix from the module path e.g., sap/ui/core/Element, When variable name "Element"
// is already in use, try with name "CoreElement"
candidateName =
pathParts[pathParts.length - 2] +
pathParts[pathParts.length - 1];
candidateName =
candidateName
.substring(0, 1)
.toUpperCase() +
candidateName.substring(1);
existingModulePath =
defineCall.getImportByParamName(
candidateName
);
}

// if there's still a naming conflict, a digit is added to the end of the candidate name till a non-conflict
// name is found
let suffix = 0;
while (
existingModulePath &&
existingModulePath !==
oModuleInner.newModulePath
) {
existingModulePath =
defineCall.getImportByParamName(
candidateName + suffix++
);
}

if (suffix !== 0) {
candidateName = candidateName + suffix;
}
}
}

const oFinder: Finder = finder[oModuleInner.finder];
const oResult: FinderResult = oFinder.find(
oNode,
Expand All @@ -217,6 +282,7 @@ function isFoundInConfig(
configName: oResult.configName,
oModuleInner,
newModulePath: oModuleInner.newModulePath,
uniqueVariableName: candidateName,
});
}
}
Expand All @@ -236,6 +302,7 @@ interface FoundReplacement {
newModulePath?: string;
};
newModulePath: string;
uniqueVariableName?: string;
}

interface ConfigObject {
Expand Down Expand Up @@ -375,6 +442,8 @@ async function migrate(args: Mod.MigrateArguments): Promise<boolean> {
if (!args.config.replacers) {
throw new Error("No replacers configured");
}

// REPLACER - code modification
for (const replacerName in args.config.replacers) {
if (args.config.replacers.hasOwnProperty(replacerName)) {
const modulePath = path.join(
Expand All @@ -392,6 +461,8 @@ async function migrate(args: Mod.MigrateArguments): Promise<boolean> {
if (!args.config.extenders) {
throw new Error("No extenders configured");
}

// EXTENDER - modify sap.ui.define dependencies
for (const extenderName in args.config.extenders) {
if (args.config.extenders.hasOwnProperty(extenderName)) {
const modulePath = path.join(
Expand Down Expand Up @@ -419,7 +490,9 @@ async function migrate(args: Mod.MigrateArguments): Promise<boolean> {
// Try to replace the call
try {
// retrieve variable name from existing import and use it as name argument of replace call
let variableNameToUse = oReplace.config.newVariableName;
let variableNameToUse =
oReplace.importObj.newVariableName ||
oReplace.config.newVariableName;
if (oReplace.config.newModulePath) {
variableNameToUse =
analyseResult.defineCall.getParamNameByImport(
Expand All @@ -437,7 +510,11 @@ async function migrate(args: Mod.MigrateArguments): Promise<boolean> {
// modify define call
const oExtender: Extender =
mExtenderFuncs[oReplace.importObj.extenderName];
oExtender.extend(analyseResult.defineCall, oReplace.config);
oExtender.extend(
analyseResult.defineCall,
oReplace.config,
oReplace.importObj.newVariableName
);

args.reporter.collect("replacementsPerformed", 1);
if (
Expand Down
5 changes: 3 additions & 2 deletions src/tasks/helpers/extenders/AddImport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,12 @@ class AddImport implements Extender {
config: {
newModulePath: string;
newVariableName: string;
}
},
name: string
): boolean {
return defineCall.addDependency(
config.newModulePath,
config.newVariableName
name || config.newVariableName
);
}
}
Expand Down
6 changes: 6 additions & 0 deletions src/tasks/helpers/replacers/Module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,12 @@ const replaceable: ASTReplaceable = {
case Syntax.ReturnStatement: // return MyModule.myField
oInsertionPoint[node.name] = oNodeModule;
break;
case Syntax.ExpressionStatement:
oInsertionPoint[node.name] = oNodeModule;
break;
case Syntax.IfStatement:
oInsertionPoint[node.name] = oNodeModule;
break;
default:
throw new Error(
"Module: insertion is of an unsupported type " +
Expand Down
26 changes: 26 additions & 0 deletions test/addMissingDependencies/extenders/nameClash.config.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
{
"modules": {
"GLOBALS": {
"*.control": {
"newModulePath": "sap/ui/core/Element",
"newVariableName": "Element",
"replacer": "Module",
"finder": "JQueryControlCallFinder",
"extender": "AddImport"
}
}
},
"finders": {
"JQueryControlCallFinder": "tasks/helpers/finders/JQueryControlCallFinder.js"
},
"extenders": {
"AddImport": "tasks/helpers/extenders/AddImport.js"
},
"replacers": {
"Module": "tasks/helpers/replacers/Module.js"
},
"comments": {
"unhandledReplacementComment": "TODO unhandled replacement"
},
"excludes": []
}
43 changes: 43 additions & 0 deletions test/addMissingDependencies/extenders/nameClash.expected.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*!
* ${copyright}
*/

// A module
sap.ui.define(["sap/me/Element", "sap/ui/core/Element"],
function(Element, CoreElement) {
"use strict";

/**
*
* @type {{}}
*/
var A = {};

/**
*
* @param oParam
* @param sContent
*/
A.x = function (oParam, iIndex) {
Element.foo();
if (CoreElement) {
var sKey = "Test." + iconName + oParam.control;
if (iconInfo.resourceBundle.hasText(sKey)) {
CoreElement;
}
var x$ = sKey();
var oTestControl = CoreElement;
A.controls = x$.control();
A.control = CoreElement;
A.controlAtPlace = CoreElement;
A.defaultControl = CoreElement;

var oActionControl = oParam.getAction().control(this.oView);
oActionControl.pause();

return oTestControl;
}
};

return A;
}, /* bExport= */ true);
43 changes: 43 additions & 0 deletions test/addMissingDependencies/extenders/nameClash.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*!
* ${copyright}
*/

// A module
sap.ui.define(["sap/me/Element"],
function(Element) {
"use strict";

/**
*
* @type {{}}
*/
var A = {};

/**
*
* @param oParam
* @param sContent
*/
A.x = function (oParam, iIndex) {
Element.foo();
if (oParam.control(0)) {
var sKey = "Test." + iconName + oParam.control;
if (iconInfo.resourceBundle.hasText(sKey)) {
$(sKey).control()[0];
}
var x$ = sKey();
var oTestControl = x$.find(".abc").children().eq(0).control(2, true);
A.controls = x$.control();
A.control = oTestControl.$().control(0);
A.controlAtPlace = x$.control(iIndex);
A.defaultControl = x$.find(".abc").children().eq(0).control()[0];

var oActionControl = oParam.getAction().control(this.oView);
oActionControl.pause();

return oTestControl;
}
};

return A;
}, /* bExport= */ true);
20 changes: 20 additions & 0 deletions test/addMissingDependenciesTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -516,5 +516,25 @@ describe("addMissingDependencies", () => {
[]
);
});

it("should add new import without name clash", done => {
const subDir = rootDir + "extenders/";
const expectedContent = fs.readFileSync(
subDir + "nameClash.expected.js",
"utf8"
);
const config = JSON.parse(
fs.readFileSync(subDir + "nameClash.config.json", "utf8")
);
const module = new CustomFileInfo(subDir + "nameClash.js");
analyseMigrateAndTest(
module,
true,
expectedContent,
config,
done,
[]
);
});
});
});