Skip to content

Commit

Permalink
fix: Handle relative imports within framework libs
Browse files Browse the repository at this point in the history
  • Loading branch information
d3xter666 authored and matz3 committed Feb 14, 2025
1 parent 9306d8a commit b063e4d
Show file tree
Hide file tree
Showing 10 changed files with 155 additions and 20 deletions.
47 changes: 35 additions & 12 deletions src/linter/ui5Types/SourceFileLinter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
getPropertyAssignmentsInObjectLiteralExpression,
findClassMember,
isClassMethod,
findAmbientModuleByDeclarationName,
} from "./utils.js";
import {taskStart} from "../../utils/perf.js";
import {getPositionsForNode} from "../../utils/nodePosition.js";
Expand Down Expand Up @@ -1439,11 +1440,13 @@ export default class SourceFileLinter {
}

const extractVarName = (node: ts.PropertyAccessExpression | ts.ElementAccessExpression) => {
const nodeName = ts.isPropertyAccessExpression(node) ?
node.name.text :
node.argumentExpression.getText();

return nodeName.replaceAll("\"", "");
if (ts.isPropertyAccessExpression(node)) {
return node.name.text;
} else if (ts.isStringLiteralLike(node.argumentExpression)) {
return node.argumentExpression.text;
} else {
return node.argumentExpression.getText();
}
};

let exprNode = node.expression;
Expand All @@ -1453,13 +1456,35 @@ export default class SourceFileLinter {
namespace.unshift(extractVarName(exprNode));
exprNode = exprNode.expression;
}
const exprType = this.checker.getTypeAtLocation(exprNode);
const potentialLibImport = exprType.symbol?.getName().replaceAll("\"", "") ?? "";
const exprTypeSymbol = this.checker.getTypeAtLocation(exprNode)?.symbol;
let potentialLibImport = "";
if (exprTypeSymbol?.valueDeclaration && ts.isModuleDeclaration(exprTypeSymbol.valueDeclaration)) {
potentialLibImport = exprTypeSymbol.valueDeclaration.name.text;
}

// Checks if the left hand side is a library import.
// It's sufficient just to check for "/library" at the end of the string by convention
if (!potentialLibImport.endsWith("/library")) {
return;
// Fallback in case of relative imports within framework libraries, where the type does not have a symbol
const exprSymbol = this.checker.getSymbolAtLocation(exprNode);
const exprDeclaration = exprSymbol?.declarations?.[0];
if (!exprDeclaration) {
return;
}
if (!ts.isImportDeclaration(exprDeclaration.parent)) {
return;
}
const moduleSpecifier = exprDeclaration.parent.moduleSpecifier;
const moduleSpecifierSymbol = this.checker.getSymbolAtLocation(moduleSpecifier);
const moduleName = moduleSpecifierSymbol?.valueDeclaration?.getSourceFile().fileName;
if (
!moduleName?.startsWith("/resources/") ||
!(moduleName?.endsWith("/library.js") || moduleName?.endsWith("/library.ts"))
) {
return;
}
// Cut off "/resources/" (11 chars) and the ".(js|ts)" extension (3 chars)
potentialLibImport = moduleName.slice(11, -3);
}

const varName = extractVarName(node);
Expand All @@ -1471,10 +1496,8 @@ export default class SourceFileLinter {

// Check if the module is registered within ambient modules
const ambientModules = this.checker.getAmbientModules();
const libAmbientModule = ambientModules.find((module) =>
module.name === `"${potentialLibImport}"`);
const isRegisteredAsUi5Module = ambientModules.some((module) =>
module.name === `"${moduleName}"`);
const libAmbientModule = findAmbientModuleByDeclarationName(ambientModules, potentialLibImport);
const isRegisteredAsUi5Module = !!findAmbientModuleByDeclarationName(ambientModules, moduleName);

let isModuleExported = true;
let libExports = libAmbientModule?.exports ?? new Map();
Expand Down
11 changes: 11 additions & 0 deletions src/linter/ui5Types/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,3 +176,14 @@ export function getPropertyAssignmentsInObjectLiteralExpression(
}
return properties;
}

export function findAmbientModuleByDeclarationName(
ambientModules: ts.Symbol[], moduleName: string
): ts.Symbol | undefined {
return ambientModules.find((symbol) => {
if (!symbol.valueDeclaration || !ts.isModuleDeclaration(symbol.valueDeclaration)) {
return false;
}
return symbol.valueDeclaration.name.text === moduleName;
});
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
sap.ui.define([
// This file covers the case where a framework library contains a relative import
// to a library module from which an implicit global is accessed.
"./library"
], function (Library) {
"use strict";

var ColorPickerMode = Library['ColorPickerMode'],

// This is a special case as ColorPickerDisplayMode is defined in its own module but also exported via the library module.
// However, this export from the library module is not reflected in the types / api.json, so it is still reported as implicit global usage.
ColorPickerDisplayMode = Library['ColorPickerDisplayMode'];
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*!
* ${copyright}
*/

/**
* Initialization Code and shared classes of library sap.ui.unified.
*/
sap.ui.define(
[
"sap/ui/core/Lib",
"sap/ui/unified/ColorPickerDisplayMode",
],
function (
Library,
ColorPickerDisplayMode,
) {
"use strict";

/**
* Unified controls intended for both, mobile and desktop scenarios
*
* @namespace
* @alias sap.ui.unified
* @author SAP SE
* @version ${version}
* @since 1.28
* @public
*/
var thisLib = Library.init({
name: "sap.ui.unified",
apiVersion: 2,
version: "${version}",
dependencies: ["sap.ui.core"],
designtime: "sap/ui/unified/designtime/library.designtime",
types: [
"sap.ui.unified.ColorPickerDisplayMode",
],
});

// expose imported enum as property of library namespace, for documentation see ColorPickerDisplayMode.js
thisLib.ColorPickerDisplayMode = ColorPickerDisplayMode;

return thisLib;
}
);
5 changes: 3 additions & 2 deletions test/fixtures/linter/rules/NoGlobals/NoExportedLibValues.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,11 @@ sap.ui.define(

var CalendarDayType = unifiedLibrary.CalendarDayType,
DateRange = unifiedLibrary["DateRange"],
DateRange2 = unifiedLibrary['DateRange'],
DateTypeRange = unifiedLibrary.DateTypeRange,
DOMAttribute = coreLibrary.tmpl.DOMAttribute,
DOMAttribute2 = coreLibrary["tmpl"].DOMAttribute,
DOMAttribute3 = coreLibrary["tmpl"]["DOMAttribute"],
DOMAttribute4 = coreLibrary.tmpl["DOMAttribute"];
DOMAttribute3 = coreLibrary['tmpl']["DOMAttribute"],
DOMAttribute4 = coreLibrary.tmpl['DOMAttribute'];
}
);
11 changes: 11 additions & 0 deletions test/lib/linter/linter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,17 @@ test.serial("lint: All files of library with sap.ui.suite namespace", async (t)
t.snapshot(preprocessLintResultsForSnapshot(res));
});

test.serial("lint: All files of library with sap.ui.unified namespace", async (t) => {
const projectPath = path.join(fixturesProjectsPath, "sap.ui.unified");
const {lintProject} = t.context;

const res = await lintProject({
rootDir: projectPath,
}, t.context.sharedLanguageService);

t.snapshot(preprocessLintResultsForSnapshot(res));
});

test.serial("lint: All files of com.ui5.troublesome.app with custom config", async (t) => {
const projectPath = path.join(fixturesProjectsPath, "com.ui5.troublesome.app");
const {lintProject} = t.context;
Expand Down
20 changes: 14 additions & 6 deletions test/lib/linter/rules/snapshots/NoGlobals.ts.md
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ Generated by [AVA](https://avajs.dev).
[
{
coverageInfo: [],
errorCount: 6,
errorCount: 7,
fatalErrorCount: 0,
filePath: 'NoExportedLibValues.js',
messages: [
Expand All @@ -401,40 +401,48 @@ Generated by [AVA](https://avajs.dev).
severity: 2,
},
{
column: 35,
column: 32,
line: 8,
message: 'Access of module \'sap/ui/unified/DateRange\' (unifiedLibrary.DateRange) not exported by library \'sap/ui/unified/library\'',
messageDetails: 'Please import the module itself directly instead of accessing it via the library module.',
ruleId: 'no-implicit-globals',
severity: 2,
},
{
column: 35,
line: 9,
message: 'Access of module \'sap/ui/unified/DateTypeRange\' (unifiedLibrary.DateTypeRange) not exported by library \'sap/ui/unified/library\'',
messageDetails: 'Please import the module itself directly instead of accessing it via the library module.',
ruleId: 'no-implicit-globals',
severity: 2,
},
{
column: 36,
line: 9,
line: 10,
message: 'Access of module \'sap/ui/core/tmpl/DOMAttribute\' (coreLibrary.tmpl.DOMAttribute) not exported by library \'sap/ui/core/library\'',
messageDetails: 'Please import the module itself directly instead of accessing it via the library module.',
ruleId: 'no-implicit-globals',
severity: 2,
},
{
column: 40,
line: 10,
line: 11,
message: 'Access of module \'sap/ui/core/tmpl/DOMAttribute\' (coreLibrary.tmpl.DOMAttribute) not exported by library \'sap/ui/core/library\'',
messageDetails: 'Please import the module itself directly instead of accessing it via the library module.',
ruleId: 'no-implicit-globals',
severity: 2,
},
{
column: 40,
line: 11,
line: 12,
message: 'Access of module \'sap/ui/core/tmpl/DOMAttribute\' (coreLibrary.tmpl.DOMAttribute) not exported by library \'sap/ui/core/library\'',
messageDetails: 'Please import the module itself directly instead of accessing it via the library module.',
ruleId: 'no-implicit-globals',
severity: 2,
},
{
column: 37,
line: 12,
line: 13,
message: 'Access of module \'sap/ui/core/tmpl/DOMAttribute\' (coreLibrary.tmpl.DOMAttribute) not exported by library \'sap/ui/core/library\'',
messageDetails: 'Please import the module itself directly instead of accessing it via the library module.',
ruleId: 'no-implicit-globals',
Expand Down
Binary file modified test/lib/linter/rules/snapshots/NoGlobals.ts.snap
Binary file not shown.
23 changes: 23 additions & 0 deletions test/lib/linter/snapshots/linter.ts.md
Original file line number Diff line number Diff line change
Expand Up @@ -3055,6 +3055,29 @@ Generated by [AVA](https://avajs.dev).
},
]

## lint: All files of library with sap.ui.unified namespace

> Snapshot 1
[
{
coverageInfo: [],
errorCount: 1,
fatalErrorCount: 0,
filePath: 'src/sap/ui/unified/ColorPicker.js',
messages: [
{
column: 36,
line: 12,
message: 'Access of module \'sap/ui/unified/ColorPickerDisplayMode\' (Library.ColorPickerDisplayMode) not exported by library \'sap/ui/unified/library\'',
ruleId: 'no-implicit-globals',
severity: 2,
},
],
warningCount: 0,
},
]

## lint: All files of com.ui5.troublesome.app with custom config

> Snapshot 1
Expand Down
Binary file modified test/lib/linter/snapshots/linter.ts.snap
Binary file not shown.

0 comments on commit b063e4d

Please sign in to comment.