Skip to content

Commit

Permalink
show a warning message.
Browse files Browse the repository at this point in the history
  • Loading branch information
Jason3S committed Dec 6, 2023
1 parent f52da81 commit f58c823
Show file tree
Hide file tree
Showing 6 changed files with 82 additions and 33 deletions.
4 changes: 4 additions & 0 deletions packages/cspell-lib/api/api.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -965,6 +965,10 @@ interface ResolveFileResult {
* A warning message if the file was found, but there was a problem.
*/
warning?: string;
/**
* The method used to resolve the file.
*/
method: string;
}
/**
* Resolve filename to absolute paths.
Expand Down
32 changes: 25 additions & 7 deletions packages/cspell-lib/src/lib/util/resolveFile.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,21 +79,39 @@ describe('Validate resolveFile', () => {
expect(r.found).toBe(found);
});

const urlIssue5034 = new URL('issue-5034/', issuesFolderURL);
const urlIssue5034 = new URL('issue-5034/.cspell.json', issuesFolderURL);

test.each`
filename | relativeTo | expected | found
${'./frontend/cfg/cspell.config.yaml'} | ${urlIssue5034.href} | ${sm(/cfg[/\\]cspell\.config\.yaml$/)} | ${true}
${'./frontend/cfg/cspell.config.yaml'} | ${new URL('cspell.json', urlIssue5034).href} | ${sm(/cfg[/\\]cspell\.config\.yaml$/)} | ${true}
${'./frontend/cfg/cspell.config.yaml'} | ${fileURLToPath(urlIssue5034)} | ${sm(/cfg[/\\]cspell\.config\.yaml$/)} | ${true}
${'./frontend/node_modules/@cspell/dict-fr-fr/cspell-ext.json'} | ${urlIssue5034.href} | ${sm(/cspell-ext\.json$/)} | ${true}
filename | relativeTo | expected | found
${'./frontend/src/cspell.config.yaml'} | ${urlIssue5034.href} | ${sm(/src[/\\]cspell\.config\.yaml$/)} | ${true}
${'./frontend/src/cspell.config.yaml'} | ${new URL('cspell.json', urlIssue5034).href} | ${sm(/src[/\\]cspell\.config\.yaml$/)} | ${true}
${'./frontend/node_modules/@cspell/dict-fr-fr/cspell-ext.json'} | ${urlIssue5034.href} | ${sm(/cspell-ext\.json$/)} | ${true}
${'@cspell/dict-fr-fr'} | ${new URL('frontend/src/cspell.json', urlIssue5034).href} | ${sm(/cspell-ext\.json$/)} | ${true}
${'@cspell/dict-mnemonics'} | ${new URL('frontend/src/cspell.json', urlIssue5034).href} | ${sm(/cspell-ext\.json$/)} | ${true}
`('resolveFile $filename rel $relativeTo', async ({ filename, relativeTo, expected, found }) => {
const r = resolveFile(filename, relativeTo);
// console.error('r %o', r);
expect(r.filename).toEqual(expected);
expect(r.found).toBe(found);
expect(r.warning).toBeUndefined();
});

test.each`
filename | relativeTo | expected | found | warning | method
${'node_modules/@cspell/dict-mnemonics/cspell-ext.json'} | ${new URL('frontend/src/cspell.json', urlIssue5034).href} | ${sm(/cspell-ext\.json$/)} | ${true} | ${expect.stringContaining('node_modules')} | ${'tryLegacyResolve'}
${'@cspell/dict-mnemonics'} | ${new URL('frontend/src/cspell.json', urlIssue5034).href} | ${sm(/cspell-ext\.json$/)} | ${true} | ${undefined} | ${'tryCreateRequire'}
${'node_modules/@cspell/dict-mnemonics'} | ${new URL('frontend/src/cspell.json', urlIssue5034).href} | ${sm(/cspell-ext\.json$/)} | ${true} | ${expect.stringContaining('node_modules')} | ${'tryLegacyResolve'}
`(
'resolveFile $filename rel $relativeTo with warning',
async ({ filename, relativeTo, expected, found, warning, method }) => {
const r = resolveFile(filename, relativeTo);
// console.error('r %o', r);
expect(r.filename).toEqual(expected);
expect(r.found).toBe(found);
expect(r.warning).toEqual(warning);
expect(r.method).toEqual(method);
},
);

test.each`
url | expected
${'/User/home'} | ${false}
Expand Down
68 changes: 44 additions & 24 deletions packages/cspell-lib/src/lib/util/resolveFile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,13 @@ export interface ResolveFileResult {
* A warning message if the file was found, but there was a problem.
*/
warning?: string;
/**
* The method used to resolve the file.
*/
method: string;
}

const testNodeModules = /^node_modules\//;
const regExpStartsWidthNodeModules = /^node_modules[/\\]/;

/**
* Resolve filename to absolute paths.
Expand All @@ -41,6 +45,19 @@ const testNodeModules = /^node_modules\//;
* @param relativeTo absolute path
*/
export function resolveFile(filename: string, relativeTo: string | URL): ResolveFileResult {
const result = _resolveFile(filename, relativeTo);
const match = filename.match(regExpStartsWidthNodeModules);

if (match) {
result.warning ??= `Import of '${filename}' should not start with '${match[0]}' in '${toFilePathOrHref(
relativeTo,
)}'. Use '${filename.replace(regExpStartsWidthNodeModules, '')}' or a relative path instead.`;
}

return result;
}

export function _resolveFile(filename: string, relativeTo: string | URL): ResolveFileResult {
filename = filename.replace(/^~/, os.homedir());
const steps: { filename: string; fn: (f: string, r: string | URL) => ResolveFileResult | undefined }[] = [
{ filename, fn: tryUrl },
Expand All @@ -50,7 +67,6 @@ export function resolveFile(filename: string, relativeTo: string | URL): Resolve
{ filename, fn: tryResolveExists },
{ filename, fn: tryNodeResolveDefaultPaths },
{ filename, fn: tryResolveFrom },
{ filename: filename.replace(testNodeModules, ''), fn: tryResolveFrom },
{ filename, fn: tryResolveGlobal },
{ filename, fn: tryLegacyResolve },
];
Expand All @@ -60,15 +76,14 @@ export function resolveFile(filename: string, relativeTo: string | URL): Resolve
if (r?.found) return r;
}

const r = tryUrl(filename, relativeTo);
const result = tryUrl(filename, relativeTo) || {
filename: isRelative(filename) ? joinWith(filename, relativeTo) : filename.toString(),
relativeTo: relativeTo.toString(),
found: false,
method: 'not found',
};

return (
r || {
filename: isRelative(filename) ? joinWith(filename, relativeTo) : filename.toString(),
relativeTo: relativeTo.toString(),
found: false,
}
);
return result;
}

/**
Expand All @@ -86,9 +101,10 @@ function tryUrl(filename: string, relativeToURL: string | URL): ResolveFileResul
filename: file,
relativeTo: undefined,
found: fs.existsSync(file),
method: 'tryUrl',
};
}
return { filename: filename.toString(), relativeTo: undefined, found: true };
return { filename: filename.toString(), relativeTo: undefined, found: true, method: 'tryUrl' };
}

if (isURLLike(relativeToURL) && !isDataURL(relativeToURL)) {
Expand All @@ -99,6 +115,7 @@ function tryUrl(filename: string, relativeToURL: string | URL): ResolveFileResul
filename: toFilePathOrHref(url),
relativeTo: toFilePathOrHref(relToURL),
found: !isRelToAFile || fs.existsSync(url),
method: 'tryUrl',
};
}

Expand All @@ -110,7 +127,7 @@ function tryCreateRequire(filename: string | URL, relativeTo: string | URL): Res
const require = createRequire(relativeTo);
try {
const r = require.resolve(filename);
return { filename: r, relativeTo: relativeTo.toString(), found: true };
return { filename: r, relativeTo: relativeTo.toString(), found: true, method: 'tryCreateRequire' };
} catch (_) {
return undefined;
}
Expand All @@ -119,7 +136,7 @@ function tryCreateRequire(filename: string | URL, relativeTo: string | URL): Res
function tryNodeResolveDefaultPaths(filename: string): ResolveFileResult | undefined {
try {
const r = require.resolve(filename);
return { filename: r, relativeTo: undefined, found: true };
return { filename: r, relativeTo: undefined, found: true, method: 'tryNodeResolveDefaultPaths' };
} catch (_) {
return undefined;
}
Expand All @@ -143,7 +160,7 @@ function tryNodeRequireResolve(filenameOrURL: string, relativeTo: string | URL):
const paths = calcPaths(path.resolve(relativeToPath));
try {
const r = require.resolve(filename, { paths });
return { filename: r, relativeTo: relativeToPath, found: true };
return { filename: r, relativeTo: relativeToPath, found: true, method: 'tryNodeRequireResolve' };
} catch (_) {
return undefined;
}
Expand All @@ -153,15 +170,15 @@ function tryImportResolve(filename: string, relativeTo: string | URL): ResolveFi
try {
const paths = isRelative(filename) ? [relativeTo] : [relativeTo, srcDirectory];
const resolved = fileURLToPath(importResolveModuleName(filename, paths));
return { filename: resolved, relativeTo: relativeTo.toString(), found: true };
return { filename: resolved, relativeTo: relativeTo.toString(), found: true, method: 'tryImportResolve' };
} catch (_) {
return undefined;
}
}

function tryResolveGlobal(filename: string): ResolveFileResult | undefined {
const r = resolveGlobal(filename);
return (r && { filename: r, relativeTo: undefined, found: true }) || undefined;
return (r && { filename: r, relativeTo: undefined, found: true, method: 'tryResolveGlobal' }) || undefined;
}

function tryResolveExists(filename: string | URL, relativeTo: string | URL): ResolveFileResult | undefined {
Expand All @@ -174,28 +191,32 @@ function tryResolveExists(filename: string | URL, relativeTo: string | URL): Res
const toTry = [{ filename }, { filename: path.resolve(relativeTo, filename), relativeTo }];
for (const { filename, relativeTo } of toTry) {
const found = path.isAbsolute(filename) && fs.existsSync(filename);
if (found) return { filename, relativeTo: relativeTo?.toString(), found };
if (found) return { filename, relativeTo: relativeTo?.toString(), found, method: 'tryResolveExists' };
}
filename = path.resolve(filename);
return {
filename,
relativeTo: path.resolve('.'),
found: fs.existsSync(filename),
method: 'tryResolveExists',
};
}

function tryResolveFrom(filename: string, relativeTo: string | URL): ResolveFileResult | undefined {
if (relativeTo instanceof URL) return undefined;
try {
return { filename: resolveFrom(pathFromRelativeTo(relativeTo), filename), relativeTo, found: true };
return {
filename: resolveFrom(pathFromRelativeTo(relativeTo), filename),
relativeTo,
found: true,
method: 'tryResolveFrom',
};
} catch (error) {
// Failed to resolve a relative module request
return undefined;
}
}

const regExpStartsWidthNodeModules = /^node_modules[/\\]/;

function tryLegacyResolve(filename: string | URL, relativeTo: string | URL): ResolveFileResult | undefined {
if (filename instanceof URL || isURLLike(filename) || (isURLLike(relativeTo) && !isFileURL(relativeTo))) {
return undefined;
Expand All @@ -206,11 +227,10 @@ function tryLegacyResolve(filename: string | URL, relativeTo: string | URL): Res
const match = filename.match(regExpStartsWidthNodeModules);

if (match) {
const found = tryImportResolve(filename.replace(regExpStartsWidthNodeModules, ''), relativeToPath);
const fixedFilename = filename.replace(regExpStartsWidthNodeModules, '');
const found = tryImportResolve(fixedFilename, relativeToPath) || tryResolveFrom(fixedFilename, relativeToPath);
if (found?.found) {
found.warning = `Import of '${filename}' should not start with '${match[0]}' in '${toFilePathOrHref(
relativeTo,
)}'`;
found.method = 'tryLegacyResolve';
return found;
}
}
Expand Down
7 changes: 7 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion test-fixtures/issues/issue-5034/frontend/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@
"name": "issue-5034-frontend",
"version": "2.34.4",
"private": true,
"dependencies": {},
"scripts": {
"spellcheck": "cspell -c \"../.cspell.json\" -r \"..\" --dot --gitignore \"**/*.{js,jsx,ts,tsx,json,yml,md}\"",
"test": "pnpm run spellcheck"
},
"devDependencies": {
"@cspell/dict-fr-fr": "2.2.2",
"@cspell/dict-mnemonics": "^3.0.1",
"cspell": "workspace:*"
}
}
2 changes: 1 addition & 1 deletion test-fixtures/issues/issue-5034/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
"private": true,
"dependencies": {},
"scripts": {
"test": "pnpm run -r spellcheck"
"test": "pnpm run -r --filter ./frontend spellcheck"
},
"devDependencies": {
"cspell": "workspace:*"
Expand Down

0 comments on commit f58c823

Please sign in to comment.