Skip to content

Commit

Permalink
fix: Correctly handle namespace resolution in linting (#367)
Browse files Browse the repository at this point in the history
fixes: #355
  • Loading branch information
d3xter666 authored Oct 22, 2024
1 parent 0e61d30 commit 922e76b
Show file tree
Hide file tree
Showing 8 changed files with 628 additions and 62 deletions.
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ You can provide multiple glob patterns as arguments after the `ui5lint` command
**Note**: Only POSIX separators are allowed, regardless of the target platform.

```sh
> ui5lint "application/webapp/**/*.xml"
> ui5lint "webapp/**/*.xml"

UI5 linter report:

Expand Down
9 changes: 8 additions & 1 deletion src/linter/LinterContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,15 +58,22 @@ export interface TranspileResult {

export interface LinterOptions {
rootDir: string;
namespace?: string;
filePatterns?: FilePattern[];
ignorePattern?: FilePattern[];
reportCoverage?: boolean;
includeMessageDetails?: boolean;
configPath?: string;
ui5ConfigPath?: string;
namespace?: string;
}

export interface FSToVirtualPathOptions {
relFsBasePath: string;
virBasePath: string;
relFsBasePathTest?: string;
virBasePathTest?: string;
};

export interface LinterParameters {
workspace: AbstractAdapter;
filePathsWorkspace: AbstractAdapter;
Expand Down
21 changes: 11 additions & 10 deletions src/linter/lintWorkspace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,37 +7,38 @@ import lintDotLibrary from "./dotLibrary/linter.js";
import lintFileTypes from "./fileTypes/linter.js";
import {taskStart} from "../utils/perf.js";
import TypeLinter from "./ui5Types/TypeLinter.js";
import LinterContext, {LintResult, LinterParameters, LinterOptions} from "./LinterContext.js";
import LinterContext, {LintResult, LinterParameters, LinterOptions, FSToVirtualPathOptions} from "./LinterContext.js";
import {createReader} from "@ui5/fs/resourceFactory";
import {resolveReader} from "./linter.js";
import {UI5LintConfigType} from "../utils/ConfigManager.js";

export default async function lintWorkspace(
workspace: AbstractAdapter, filePathsWorkspace: AbstractAdapter,
options: LinterOptions, config: UI5LintConfigType, patternsMatch: Set<string>
options: LinterOptions & FSToVirtualPathOptions, config: UI5LintConfigType, patternsMatch: Set<string>
): Promise<LintResult[]> {
const done = taskStart("Linting Workspace");
const {relFsBasePath, virBasePath, relFsBasePathTest, virBasePathTest} = options;

const context = new LinterContext(options);
let reader = await resolveReader({
let reader = resolveReader({
patterns: options.filePatterns ?? config.files ?? [],
projectRootDir: options.rootDir,
ui5ConfigPath: config.ui5Config,
relFsBasePath: relFsBasePath ?? "",
virBasePath: virBasePath ?? "/",
relFsBasePathTest, virBasePathTest,
resourceReader: createReader({
fsBasePath: options.rootDir,
virBasePath: "/",
}),
inverseResult: true,
namespace: options.namespace,
patternsMatch,
});
reader = await resolveReader({
reader = resolveReader({
patterns: options.ignorePattern ?? [],
projectRootDir: options.rootDir,
ui5ConfigPath: config.ui5Config,
resourceReader: reader,
namespace: options.namespace,
patternsMatch,
relFsBasePath: relFsBasePath ?? "",
virBasePath: virBasePath ?? "/",
relFsBasePathTest, virBasePathTest,
});
context.setRootReader(reader);

Expand Down
76 changes: 28 additions & 48 deletions src/linter/linter.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {graphFromObject} from "@ui5/project/graph";
import {createReader, createWorkspace, createReaderCollection, createFilterReader} from "@ui5/fs/resourceFactory";
import {FilePath, FilePattern, LinterOptions, LintResult} from "./LinterContext.js";
import {FilePath, FilePattern, LinterOptions, FSToVirtualPathOptions, LintResult} from "./LinterContext.js";
import lintWorkspace from "./lintWorkspace.js";
import {taskStart} from "../utils/perf.js";
import path from "node:path";
Expand Down Expand Up @@ -59,6 +59,9 @@ export async function lintProject({
});
}

const relFsBasePath = path.relative(rootDir, fsBasePath);
const relFsBasePathTest = fsBasePathTest ? path.relative(rootDir, fsBasePathTest) : undefined;

const res = await lint(reader, {
rootDir,
namespace: project.getNamespace(),
Expand All @@ -67,11 +70,9 @@ export async function lintProject({
reportCoverage,
includeMessageDetails,
configPath,
ui5ConfigPath,
relFsBasePath, virBasePath, relFsBasePathTest, virBasePathTest,
}, config);

const relFsBasePath = path.relative(rootDir, fsBasePath);
const relFsBasePathTest = fsBasePathTest ? path.relative(rootDir, fsBasePathTest) : undefined;
res.forEach((result) => {
result.filePath = transformVirtualPathToFilePath(result.filePath,
relFsBasePath, virBasePath,
Expand All @@ -89,9 +90,10 @@ export async function lintFile({
const configMngr = new ConfigManager(rootDir, configPath);
const config = await configMngr.getConfiguration();

const virBasePath = namespace ? `/resources/${namespace}/` : "/";
const reader = createReader({
fsBasePath: rootDir,
virBasePath: namespace ? `/resources/${namespace}/` : "/",
virBasePath,
});

const res = await lint(reader, {
Expand All @@ -102,6 +104,8 @@ export async function lintFile({
reportCoverage,
includeMessageDetails,
configPath,
relFsBasePath: "",
virBasePath,
}, config);

res.forEach((result) => {
Expand All @@ -114,11 +118,12 @@ export async function lintFile({
}

async function lint(
resourceReader: AbstractReader, options: LinterOptions, config: UI5LintConfigType
resourceReader: AbstractReader, options: LinterOptions & FSToVirtualPathOptions,
config: UI5LintConfigType
): Promise<LintResult[]> {
const lintEnd = taskStart("Linting");
let {ignorePattern, filePatterns} = options;
const {rootDir} = options;
const {relFsBasePath, virBasePath, relFsBasePathTest, virBasePathTest} = options;

// Resolve files to include
filePatterns = filePatterns ?? config.files ?? [];
Expand All @@ -131,29 +136,26 @@ async function lint(
// Apply ignores to the workspace reader.
// TypeScript needs the full context to provide correct analysis.
// so, we can do filtering later via the filePathsReader
const reader = await resolveReader({
const reader = resolveReader({
patterns: ignorePattern,
projectRootDir: rootDir,
resourceReader,
namespace: options.namespace,
patternsMatch: matchedPatterns,
relFsBasePath, virBasePath, relFsBasePathTest, virBasePathTest,
});

// Apply files + ignores over the filePaths reader
let filePathsReader = await resolveReader({
let filePathsReader = resolveReader({
patterns: filePatterns,
projectRootDir: rootDir,
resourceReader,
inverseResult: true,
namespace: options.namespace,
patternsMatch: matchedPatterns,
relFsBasePath, virBasePath, relFsBasePathTest, virBasePathTest,
});
filePathsReader = await resolveReader({
filePathsReader = resolveReader({
patterns: ignorePattern,
projectRootDir: rootDir,
resourceReader: filePathsReader,
namespace: options.namespace,
patternsMatch: matchedPatterns,
relFsBasePath, virBasePath, relFsBasePathTest, virBasePathTest,
});
const filePathsWorkspace = createWorkspace({reader: filePathsReader});

Expand Down Expand Up @@ -297,56 +299,31 @@ function buildPatterns(patterns: string[]) {
`"${pattern}" defines an absolute path.`);
}

if (pattern.endsWith("/")) { // Match all files in a directory
pattern += "**/*";
}

return new Minimatch(pattern, {flipNegate: true});
});
}

export async function resolveReader({
export function resolveReader({
patterns,
projectRootDir,
resourceReader,
namespace,
ui5ConfigPath,
inverseResult = false,
patternsMatch,
relFsBasePath, virBasePath, relFsBasePathTest, virBasePathTest,
}: {
patterns: string[];
projectRootDir: string;
resourceReader: AbstractReader;
namespace?: string;
ui5ConfigPath?: string;
inverseResult?: boolean;
patternsMatch: Set<string>;
relFsBasePath: string; virBasePath: string; relFsBasePathTest?: string; virBasePathTest?: string;
}) {
if (!patterns.length) {
return resourceReader;
}

let fsBasePath = projectRootDir;
let fsBasePathTest = path.join(projectRootDir, "test");
let virBasePath = namespace ? `/resources/${namespace}/` : "/resources/";
let virBasePathTest = namespace ? `/test-resources/${namespace}/` : "/test-resources/";

try {
const graph = await getProjectGraph(projectRootDir, ui5ConfigPath);
const project = graph.getRoot();
projectRootDir = project.getRootPath();
fsBasePath = project.getSourcePath();
fsBasePathTest = path.join(projectRootDir, project._testPath ?? "test");

if (!namespace && !project._isSourceNamespaced) {
// Ensure the virtual filesystem includes the project namespace to allow relative imports
// of framework resources from the project
virBasePath += project.getNamespace() + "/";
virBasePathTest += project.getNamespace() + "/";
}
} catch {
// Project is not resolved i.e. in tests
}

const relFsBasePath = path.relative(projectRootDir, fsBasePath);
const relFsBasePathTest = fsBasePathTest ? path.relative(projectRootDir, fsBasePathTest) : undefined;

const minimatchPatterns = buildPatterns(patterns);

return createFilterReader({
Expand Down Expand Up @@ -374,6 +351,9 @@ export async function resolveReader({
*/
function checkUnmatchedPatterns(patterns: FilePattern[], patternsMatch: Set<string>) {
const unmatchedPatterns = patterns.reduce((acc, pattern) => {
if (pattern.endsWith("/")) { // Match all files in a directory
pattern += "**/*";
}
if (!patternsMatch.has(pattern)) {
acc.push(pattern);
}
Expand Down
81 changes: 81 additions & 0 deletions test/fixtures/linter/projects/sap.ui.core/src/ui5loader.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
/*!
* ${copyright}
*/

/*
* IMPORTANT NOTICE
* With 1.54, ui5loader.js and its new features are not yet a public API.
* The loader must only be used via the well-known and documented UI5 APIs
* such as sap.ui.define, sap.ui.require, etc.
* Any direct usage of ui5loader.js or its features is not supported and
* might break in future releases.
*/

/*global sap:true, Blob, console, document, Promise, URL, XMLHttpRequest */

(function(__global) {
"use strict";

/**
* Root namespace for JavaScript functionality provided by SAP SE.
*
* @version ${version}
* @namespace
* @public
* @name sap
*/

__global.sap = __global.sap || {};
sap.ui = sap.ui || {};

/**
* Provides access to UI5 loader configuration.
*
* The configuration is used by {@link sap.ui.require} and {@link sap.ui.define}.
*
* @public
* @namespace
* @ui5-global-only
*/
sap.ui.loader = {};

/**
* @private
* @ui5-restricted bundles created with UI5 tooling
* @function
* @ui5-global-only
*/
sap.ui.predefine = predefine;

/**
* Load a single module synchronously and return its module value.
*
* Basically, this method is a combination of {@link jQuery.sap.require} and {@link sap.ui.require}.
* Its main purpose is to simplify the migration of modules to AMD style in those cases where some dependencies
* have to be loaded late (lazy) and synchronously.
*
* The method accepts a single module name in the same syntax that {@link sap.ui.define} and {@link sap.ui.require}
* already use (a simplified variation of the {@link jQuery.sap.getResourcePath unified resource name}:
* slash separated names without the implicit extension '.js'). As for <code>sap.ui.require</code>,
* relative names (using <code>./</code> or <code>../</code>) are not supported.
* If not loaded yet, the named module will be loaded synchronously and the export value of the module will be returned.
* While a module is executing, a value of <code>undefined</code> will be returned in case it is required again during
* that period of time (e.g. in case of cyclic dependencies).
*
* <b>Note:</b> the scope of this method is limited to the sap.ui.core library. Callers are strongly encouraged to use
* this method only when synchronous loading is unavoidable. Any code that uses this method won't benefit from future
* performance improvements that require asynchronous module loading (e.g. HTTP/2). And such code never can comply with
* a content security policies (CSP) that forbids 'eval'.
*
* @param {string} sModuleName Module name in requireJS syntax
* @returns {any} Export value of the loaded module (can be <code>undefined</code>)
* @private
* @ui5-restricted sap.ui.core
* @function
* @ui5-global-only
* @deprecated As of version 1.120, sync loading is deprecated without replacement due to the deprecation
* of sync XMLHttpRequests in the web standard.
*/
sap.ui.requireSync = requireSync;

}(globalThis));
34 changes: 32 additions & 2 deletions test/lib/linter/linter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,22 @@ test.serial("lint: Some files of com.ui5.troublesome.app (without details / cove
t.snapshot(preprocessLintResultsForSnapshot(res));
});

test.serial("lint: Only /webapp folder from com.ui5.troublesome.app (without details / coverage)", async (t) => {
const projectPath = path.join(fixturesProjectsPath, "com.ui5.troublesome.app");
const filePaths = [
// Minimatch requires POSIX
"webapp/",
];
const {lintProject} = t.context;

const res = await lintProject({
rootDir: projectPath,
filePatterns: filePaths,
});

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

test.serial("lint: One file of com.ui5.troublesome.app (without details / coverage)", async (t) => {
const projectPath = path.join(fixturesProjectsPath, "com.ui5.troublesome.app");
const filePaths = [
Expand Down Expand Up @@ -144,8 +160,8 @@ test.serial("lint: Ignore files from library.with.custom.paths", async (t) => {
reportCoverage: true,
includeMessageDetails: true,
ignorePattern: [
"src/**/*",
"!src/main/**/*",
"src/",
"!src/main/",
],
});

Expand Down Expand Up @@ -184,6 +200,20 @@ test.serial("lint: All files of mocked minimal sap.ui.core library", async (t) =
t.snapshot(preprocessLintResultsForSnapshot(res));
});

test.serial("lint: File out of the namespace of sap.ui.core", async (t) => {
const projectPath = path.join(fixturesProjectsPath, "sap.ui.core");
const {lintProject} = t.context;

const res = await lintProject({
rootDir: projectPath,
filePatterns: ["src/ui5loader.js"],
reportCoverage: true,
includeMessageDetails: true,
});

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

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

0 comments on commit 922e76b

Please sign in to comment.