Skip to content

Commit

Permalink
fix: Improve module resolution
Browse files Browse the repository at this point in the history
This solves issues where some detections did not work properly due to
the way dependencies were resolved.

When using relative imports, the module resolution was working, but when
for example importing a control from a test, the complete namespace is
used, but the corresponding JS file was not found.

This change ensures that JS files are resolved and also that the types
for a framework library are loaded when it is linted.
  • Loading branch information
matz3 committed Dec 10, 2024
1 parent e0c69f4 commit ce4eed8
Show file tree
Hide file tree
Showing 7 changed files with 76 additions and 72 deletions.
51 changes: 2 additions & 49 deletions src/linter/ui5Types/SourceFileLinter.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import ts, {SymbolFlags} from "typescript";
import ts from "typescript";
import path from "node:path/posix";
import {getLogger} from "@ui5/logger";
import SourceFileReporter from "./SourceFileReporter.js";
Expand Down Expand Up @@ -282,54 +282,7 @@ export default class SourceFileLinter {
!decl.getSourceFile().isDeclarationFile);

// If the original raw render file is available, we can analyze it directly
if (originalDeclarations && originalDeclarations.length > 0) {
originalDeclarations.forEach((declaration) => this.analyzeControlRendererInternals(declaration));
} else {
// When there's an ambient module, then we need special handling.
// In case we have the raw file & typescript definitions for it, their types
// are being merged, creating an ambient module. We need to find the original
// raw file in order to analyze the renderer. Such special case, is for example
// any openui5 library when we analyze the openui5 repo- we have the TS definitions
// loaded in the TS compiler via @sapui5/types, but we actually want to analyze the
// source files of those types.
let importModuleString = "";

// Get the import string module from the current source file, so we can filter later
const rendererSymbol = this.checker.getSymbolAtLocation(rendererMember.initializer);
const importedRenderModule = rendererSymbol?.getDeclarations()?.[0]?.parent;

if (importedRenderModule &&
ts.isImportDeclaration(importedRenderModule) &&
ts.isStringLiteral(importedRenderModule.moduleSpecifier)) {
importModuleString = importedRenderModule.moduleSpecifier.text;
}

// Find the correct raw source file
const exportSourceFile = this.program.getSourceFiles().find((sourceFile) =>
sourceFile.fileName.includes(importModuleString));

// Extract the exports from the source file
const exportFileSymbol = exportSourceFile && this.checker.getSymbolAtLocation(exportSourceFile);
const moduleExportsSymbols = exportFileSymbol && this.checker.getExportsOfModule(exportFileSymbol);

// Check all exports
moduleExportsSymbols?.forEach((exportSymbol) => {
// Note: getAliasedSymbol fails when called with a symbol that isn't an alias
let symbol = exportSymbol;
if (exportSymbol.flags & SymbolFlags.TypeAlias) {
// Export could be a "default", so we need to get the real reference of the export symbol
symbol = this.checker.getAliasedSymbol(exportSymbol);
}

symbol?.getDeclarations()?.forEach((declaration) => {
if (ts.isVariableDeclaration(declaration) && declaration.initializer) {
this.analyzeControlRendererInternals(declaration.initializer);
} else if (ts.isExportAssignment(declaration) && declaration.expression) {
this.analyzeControlRendererInternals(declaration.expression);
}
});
});
}
originalDeclarations?.forEach((declaration) => this.analyzeControlRendererInternals(declaration));
} else {
// Analyze renderer property when it's directly embedded in the renderer object
// i.e. { renderer: {apiVersion: "2", render: () => {}} }
Expand Down
7 changes: 6 additions & 1 deletion src/linter/ui5Types/TypeLinter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,12 @@ export default class TypeChecker {
if (namespace) {
// Map namespace used in imports (without /resources) to /resources paths
this.#compilerOptions.paths = {
[`${namespace}/*`]: [`/resources/${namespace}/*`],
[`${namespace}/*`]: [
// Enforce that the compiler also tries to resolve imports with a .js extension.
// With this mapping, the compiler still first tries to resolve the .ts, .tsx and .d.ts extensions
// but then falls back to .js
`/resources/${namespace}/*.js`,
],
};
}
}
Expand Down
41 changes: 24 additions & 17 deletions src/linter/ui5Types/host.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,17 +47,26 @@ async function collectSapui5TypesFiles() {
return typesFiles;
}

function addSapui5TypesMappingToCompilerOptions(sapui5TypesFiles: string[], options: ts.CompilerOptions) {
function addSapui5TypesMappingToCompilerOptions(
sapui5TypesFiles: string[], options: ts.CompilerOptions, projectNamespace?: string
) {
const paths = options.paths ?? (options.paths = {});
sapui5TypesFiles.forEach((fileName) => {
if (fileName === "sap.ui.core.d.ts") {
// No need to add a mapping for sap.ui.core, as it is loaded by default
return;
}
const dtsPath = `/types/@sapui5/types/types/${fileName}`;
const libraryName = posixPath.basename(fileName, ".d.ts");
const namespace = libraryName.replace(/\./g, "/") + "/*";
const pathsEntry = paths[namespace] ?? (paths[namespace] = []);
pathsEntry.push(`/types/@sapui5/types/types/${fileName}`);
const libraryNamespace = libraryName.replace(/\./g, "/");
if (libraryNamespace === "sap/ui/core" || libraryNamespace === projectNamespace) {
// Special cases:
// - sap.ui.core needs to be loaded by default as it provides general API that must always be available
// - When linting a framework library, the corresponding types should be loaded by default as they
// might not get loaded by the compiler otherwise, as the actual sources are available in the project.
options.types?.push(dtsPath);
} else {
// For other framework libraries we can add a paths mapping to load them on demand
const mappingNamespace = libraryNamespace + "/*";
const pathsEntry = paths[mappingNamespace] ?? (paths[mappingNamespace] = []);
pathsEntry.push(dtsPath);
}
});
}

Expand All @@ -70,6 +79,9 @@ export async function createVirtualCompilerHost(
): Promise<ts.CompilerHost> {
const silly = log.isLevelEnabled("silly");

options.typeRoots = ["/types"];
options.types = [];

const typePathMappings = new Map<string, string>();
addPathMappingForPackage("typescript", typePathMappings);

Expand All @@ -85,16 +97,11 @@ export async function createVirtualCompilerHost(
require.resolve("../../../resources/overrides/package.json")
));

options.typeRoots = ["/types"];
options.types = [
// Request compiler to only use sap.ui.core types by default - other types will be loaded on demand
// (see addSapui5TypesMappingToCompilerOptions)
...typePackageDirs.filter((dir) => dir !== "/types/@sapui5/types/"),
"/types/@sapui5/types/types/sap.ui.core.d.ts",
];
// Add all types except @sapui5/types which will be handled below
options.types.push(...typePackageDirs.filter((dir) => dir !== "/types/@sapui5/types/"));

// Adds mappings for all other sapui5 types, so that they are only loaded once a module is imported
addSapui5TypesMappingToCompilerOptions(await collectSapui5TypesFiles(), options);
// Adds types / mappings for all @sapui5/types
addSapui5TypesMappingToCompilerOptions(await collectSapui5TypesFiles(), options, context.getNamespace());

// Create regex matching all path mapping keys
const pathMappingRegex = new RegExp(
Expand Down
12 changes: 12 additions & 0 deletions test/fixtures/linter/projects/sap.f/src/sap/f/NewControl.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
sap.ui.define([
"sap/ui/core/Control"
], function (Control) {
"use strict";
// This control is used to test cases where a framework library contains a control
// for which no @sapui5/types exist.
// The control and usages of it should still be analyzed properly, e.g. when extending it
// and not defining a renderer.
return Control.extend("sap.f.NewControl", {
renderer: null
});
});
11 changes: 7 additions & 4 deletions test/fixtures/linter/projects/sap.f/test/sap/f/LinterTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,18 @@
sap.ui.require([
"sap/f/Avatar",
"sap/m/DateTimeInput",
"sap/f/ProductSwitchRenderer"
], (Avatar, DateTimeInput, ProductSwitchRenderer) => {
"sap/f/ProductSwitchRenderer",
"sap/f/NewControl"
], (Avatar, DateTimeInput, ProductSwitchRenderer, NewControl) => {
new Avatar();
new DateTimeInput();

Avatar.extend("CustomControl", {
// Usage of render function without object is deprecated as no apiVersion is defined
// TODO detect: This is currently not detected as the module resolution is not working correctly.
// This needs to be solved in a separate change.
renderer: ProductSwitchRenderer.render
});

// This ensures that the renderer declaration is also checked for controls based on
// controls of a framework library for which no @sapui5/types exist (sap/f/NewControl).
const CustomNewControl = NewControl.extend("CustomNewControl");
});
26 changes: 25 additions & 1 deletion test/lib/linter/snapshots/linter.ts.md
Original file line number Diff line number Diff line change
Expand Up @@ -2085,6 +2085,14 @@ Generated by [AVA](https://avajs.dev).
messages: [],
warningCount: 0,
},
{
coverageInfo: [],
errorCount: 0,
fatalErrorCount: 0,
filePath: 'src/sap/f/NewControl.js',
messages: [],
warningCount: 0,
},
{
coverageInfo: [],
errorCount: 0,
Expand Down Expand Up @@ -2127,10 +2135,18 @@ Generated by [AVA](https://avajs.dev).
},
{
coverageInfo: [],
errorCount: 2,
errorCount: 4,
fatalErrorCount: 0,
filePath: 'test/sap/f/LinterTest.js',
messages: [
{
column: 13,
line: 14,
message: 'Use of deprecated renderer detected. Define explicitly the {apiVersion: 2} parameter in the renderer object',
messageDetails: '"Renderer Object (https://ui5.sap.com/#/topic/c9ab34570cc14ea5ab72a6d1a4a03e3f)",',
ruleId: 'no-deprecated-api',
severity: 2,
},
{
column: 2,
line: 4,
Expand All @@ -2147,6 +2163,14 @@ Generated by [AVA](https://avajs.dev).
ruleId: 'no-deprecated-api',
severity: 2,
},
{
column: 27,
line: 19,
message: 'Control \'CustomNewControl\' is missing a renderer declaration',
messageDetails: 'Not defining a \'renderer\' for control \'CustomNewControl\' may lead to synchronous loading of the \'CustomNewControlRenderer\' module. If no renderer exists, set \'renderer: null\'. Otherwise, either import the renderer module and assign it to the \'renderer\' property or implement the renderer inline.',
ruleId: 'no-deprecated-control-renderer-declaration',
severity: 2,
},
],
warningCount: 0,
},
Expand Down
Binary file modified test/lib/linter/snapshots/linter.ts.snap
Binary file not shown.

0 comments on commit ce4eed8

Please sign in to comment.