fix: Resolve relative imports without a leading ./ or ../. (#5035)

streetsidesoftware · Dec 6, 2023 · a28393b · a28393b
1 parent 8d481f3
commit a28393b
Show file tree

Hide file tree

Showing 14 changed files with 238 additions and 24 deletions.
diff --git a/packages/cspell-lib/api/api.d.ts b/packages/cspell-lib/api/api.d.ts
@@ -955,9 +955,20 @@ declare function setLogger(logger: Logger): Logger;
 declare function getLogger(): Logger;
 
 interface ResolveFileResult {
+    /**
+     * Absolute path or URL to the file.
+     */
     filename: string;
     relativeTo: string | undefined;
     found: boolean;
+    /**
+     * 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.

diff --git a/packages/cspell-lib/src/lib/Settings/Controller/configLoader/configLoader.test.ts b/packages/cspell-lib/src/lib/Settings/Controller/configLoader/configLoader.test.ts
@@ -1,15 +1,16 @@
 import type { CSpellSettingsWithSourceTrace, CSpellUserSettings, ImportFileRef } from '@cspell/cspell-types';
 import type { CSpellConfigFile } from 'cspell-config-lib';
 import * as path from 'path';
-import { pathToFileURL } from 'url';
-import { beforeEach, describe, expect, test, vi } from 'vitest';
+import { fileURLToPath, pathToFileURL } from 'url';
+import { assert, beforeEach, describe, expect, test, vi } from 'vitest';
 
 import {
     pathPackageRoot,
     pathPackageSamples,
     pathPackageSamplesURL,
     pathRepoRoot,
     pathRepoTestFixtures,
+    pathRepoTestFixturesURL,
 } from '../../../../test-util/test.locations.cjs';
 import { logError, logWarning } from '../../../util/logger.js';
 import { resolveFileWithURL, toFilePathOrHref, toFileUrl } from '../../../util/url.js';
@@ -29,7 +30,7 @@ import {
     readRawSettings,
     searchForConfig,
 } from './defaultConfigLoader.js';
-import { extractImportErrors } from './extractImportErrors.js';
+import { extractImportErrors, extractImports } from './extractImportErrors.js';
 import { readSettings } from './readSettings.js';
 import { readSettingsFiles } from './readSettingsFiles.js';
 
@@ -41,7 +42,10 @@ const samplesDir = pathPackageSamples;
 const samplesSrc = path.join(samplesDir, 'src');
 const testFixtures = pathRepoTestFixtures;
 
+const urlIssues = new URL('./issues/', pathRepoTestFixturesURL);
+
 const oc = expect.objectContaining;
+const sm = expect.stringMatching;
 
 vi.mock('../../../util/logger');
 
@@ -410,6 +414,20 @@ describe('Validate search/load config files', () => {
         expect(searchResult?.url.href).toEqual(expectedConfig.href);
     });
 
+    test.each`
+        dir                                                   | expectedImports
+        ${new URL('issue-5034/.cspell.json', urlIssues).href} | ${[oc({ filename: sm(/cspell-ext.json/) }), oc({ filename: sm(/.cspell.json/) })]}
+    `('Search and merge from $dir', async ({ dir, expectedImports }) => {
+        const loader = getDefaultConfigLoader();
+        const url = toFileUrl(dir);
+        const searchResult = await loader.searchForConfigFile(url);
+        assert(searchResult);
+        expect(searchResult.url.href).toEqual(url.href);
+        const settings = await loader.mergeConfigFileWithImports(searchResult);
+        expect(settings?.__importRef?.filename).toEqual(fileURLToPath(url));
+        expect(extractImports(settings)).toEqual(expectedImports);
+    });
+
     test.each`
         dir                                     | expectedConfig                                                      | expectedImportErrors
         ${pathToFileURL(samplesSrc + '/').href} | ${cfg(s('.cspell.json'))}                                           | ${[]}

diff --git a/packages/cspell-lib/src/lib/Settings/Controller/configLoader/configLoader.ts b/packages/cspell-lib/src/lib/Settings/Controller/configLoader/configLoader.ts
@@ -496,6 +496,10 @@ function resolveFilename(filename: string | URL, relativeTo: string | URL): Impo
     if (filename instanceof URL) return { filename: toFilePathOrHref(filename) };
     const r = resolveFile(filename, relativeTo);
 
+    if (r.warning) {
+        logWarning(r.warning);
+    }
+
     return {
         filename: r.filename.startsWith('file:/') ? fileURLToPath(r.filename) : r.filename,
         error: r.found ? undefined : new Error(`Failed to resolve file: "${filename}"`),

diff --git a/packages/cspell-lib/src/lib/util/resolveFile.test.ts b/packages/cspell-lib/src/lib/util/resolveFile.test.ts
@@ -5,13 +5,16 @@ import * as path from 'path';
 import { fileURLToPath, pathToFileURL } from 'url';
 import { describe, expect, test } from 'vitest';
 
+import { pathRepoTestFixturesURL } from '../../test-util/index.mjs';
 import { __testing__, resolveFile } from './resolveFile.js';
 import { toFilePathOrHref } from './url.js';
 
 interface Config {
     import: string[];
 }
 
+const issuesFolderURL = new URL('./issues/', pathRepoTestFixturesURL);
+
 const defaultConfigFile = require.resolve('@cspell/cspell-bundled-dicts/cspell-default.json');
 const defaultConfigLocation = path.dirname(defaultConfigFile);
 
@@ -26,6 +29,7 @@ const rr = {
 };
 
 const oc = expect.objectContaining;
+const sm = expect.stringMatching;
 
 const { isFileURL, tryUrl } = __testing__;
 
@@ -75,6 +79,39 @@ describe('Validate resolveFile', () => {
         expect(r.found).toBe(found);
     });
 
+    const urlIssue5034 = new URL('issue-5034/.cspell.json', issuesFolderURL);
+
+    test.each`
+        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);
+        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}

diff --git a/packages/cspell-lib/src/lib/util/resolveFile.ts b/packages/cspell-lib/src/lib/util/resolveFile.ts
@@ -20,12 +20,23 @@ import {
 } from './url.js';
 
 export interface ResolveFileResult {
+    /**
+     * Absolute path or URL to the file.
+     */
     filename: string;
     relativeTo: string | undefined;
     found: boolean;
+    /**
+     * 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.
@@ -34,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 },
@@ -43,24 +67,23 @@ 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 },
     ];
 
     for (const step of steps) {
         const r = step.fn(step.filename, relativeTo);
         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;
 }
 
 /**
@@ -78,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)) {
@@ -91,6 +115,7 @@ function tryUrl(filename: string, relativeToURL: string | URL): ResolveFileResul
             filename: toFilePathOrHref(url),
             relativeTo: toFilePathOrHref(relToURL),
             found: !isRelToAFile || fs.existsSync(url),
+            method: 'tryUrl',
         };
     }
 
@@ -102,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;
     }
@@ -111,15 +136,15 @@ 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;
     }
 }
 
 function tryNodeRequireResolve(filenameOrURL: string, relativeTo: string | URL): ResolveFileResult | undefined {
     const filename = fileURLOrPathToPath(filenameOrURL);
-    const relativeToPath = fileURLOrPathToPath(relativeTo);
+    const relativeToPath = pathFromRelativeTo(relativeTo);
     const home = os.homedir();
     function calcPaths(p: string) {
         const paths = [p];
@@ -135,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;
     }
@@ -145,43 +170,74 @@ 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 {
-    if (filename instanceof URL || isURLLike(filename) || isURLLike(relativeTo)) return undefined;
+    if (filename instanceof URL || isURLLike(filename) || (isURLLike(relativeTo) && !isFileURL(relativeTo))) {
+        return undefined;
+    }
+
+    relativeTo = pathFromRelativeTo(relativeTo);
 
-    const toTry = [{ filename }, { filename: path.resolve(relativeTo.toString(), filename), relativeTo }];
+    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(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;
     }
 }
 
+function tryLegacyResolve(filename: string | URL, relativeTo: string | URL): ResolveFileResult | undefined {
+    if (filename instanceof URL || isURLLike(filename) || (isURLLike(relativeTo) && !isFileURL(relativeTo))) {
+        return undefined;
+    }
+
+    const relativeToPath = isURLLike(relativeTo) ? fileURLToPath(new URL('./', relativeTo)) : relativeTo.toString();
+
+    const match = filename.match(regExpStartsWidthNodeModules);
+
+    if (match) {
+        const fixedFilename = filename.replace(regExpStartsWidthNodeModules, '');
+        const found = tryImportResolve(fixedFilename, relativeToPath) || tryResolveFrom(fixedFilename, relativeToPath);
+        if (found?.found) {
+            found.method = 'tryLegacyResolve';
+            return found;
+        }
+    }
+
+    return undefined;
+}
+
 function isRelative(filename: string | URL): boolean {
     if (filename instanceof URL) return false;
     if (filename.startsWith('./')) return true;
@@ -197,6 +253,10 @@ function joinWith(filename: string, relativeTo: string | URL): string {
         : path.resolve(relativeTo, filename);
 }
 
+function pathFromRelativeTo(relativeTo: string | URL): string {
+    return relativeTo instanceof URL || isURLLike(relativeTo) ? fileURLToPath(new URL('./', relativeTo)) : relativeTo;
+}
+
 export const __testing__ = {
     isRelative,
     isFileURL,