Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore field name mappings in package.json files that are not paths of existing files #46

Merged
merged 8 commits into from
Jun 24, 2018
127 changes: 87 additions & 40 deletions src/match-path-async.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import * as path from "path";
import * as TryPath from "./try-path";
import * as MappingEntry from "./mapping-entry";
import * as Filesystem from "./filesystem";
import { getPrioritizedMainFieldName } from "./match-path-sync";

/**
* Function that can match a path async
Expand Down Expand Up @@ -84,6 +83,49 @@ export function matchFromAbsolutePathsAsync(
);
}

function findFirstExistingMainFieldMappedFile(
packageJson: Filesystem.PackageJson,
mainFields: string[],
packageJsonPath: string,
fileExistsAsync: Filesystem.FileExistsAsync,
doneCallback: (err?: Error, filepath?: string) => void,
index: number = 0
): void {
if (index >= mainFields.length) {
return doneCallback(undefined, undefined);
}

const tryNext = () =>
findFirstExistingMainFieldMappedFile(
packageJson,
mainFields,
packageJsonPath,
fileExistsAsync,
doneCallback,
index + 1
);

const mainFieldMapping = packageJson[mainFields[index]];
if (typeof mainFieldMapping !== "string") {
// Skip mappings that are not pointers to replacement files
return tryNext();
}

const mappedFilePath = path.join(
path.dirname(packageJsonPath),
mainFieldMapping
);
fileExistsAsync(mappedFilePath, (err?: Error, exists?: boolean) => {
if (err) {
return doneCallback(err);
}
if (exists) {
return doneCallback(undefined, mappedFilePath);
}
return tryNext();
});
}

// Recursive loop to probe for physical files
function findFirstExistingPath(
tryPaths: ReadonlyArray<TryPath.TryPath>,
Expand Down Expand Up @@ -125,48 +167,53 @@ function findFirstExistingPath(
if (err) {
return doneCallback(err);
}
const mainFieldName = getPrioritizedMainFieldName(
packageJson,
mainFields
);
if (mainFieldName) {
const file = path.join(
path.dirname(tryPath.path),
packageJson[mainFieldName]
);
fileExists(file, (err2, exists) => {
if (err2) {
return doneCallback(err2);
}
if (exists) {
// Not sure why we don't just return the full path? Why strip it?
return doneCallback(undefined, Filesystem.removeExtension(file));
}
// Continue with the next path
return findFirstExistingPath(
tryPaths,
readJson,
fileExists,
doneCallback,
index + 1,
mainFields
);
});
} else {
// This is async code, we need to return in an else-branch otherwise the code still falls through and keeps recursing.
// While this might work in general, libraries that use neo-async like Webpack will actually not allow you to call the same callback twice.
// An example of where this caused issues: https://github.com/dividab/tsconfig-paths-webpack-plugin/issues/11
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we keep this comment somewhere or is it no longer relevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I'll include it at the bottom return statement. Thanks for pointing it out!


// Continue with the next path
return findFirstExistingPath(
tryPaths,
readJson,
if (packageJson) {
return findFirstExistingMainFieldMappedFile(
packageJson,
mainFields,
tryPath.path,
fileExists,
doneCallback,
index + 1,
mainFields
(mainFieldErr?: Error, mainFieldMappedFile?: string) => {
if (mainFieldErr) {
return doneCallback(mainFieldErr);
}
if (mainFieldMappedFile) {
// Not sure why we don't just return the full path? Why strip it?
return doneCallback(
undefined,
Filesystem.removeExtension(mainFieldMappedFile)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the comment above asks, why are we actually removing the extension?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC I added this comment some time ago when doing the initial work on the async version. @Jontem do you remember why the stripping is done? Or do you also think we can just return the raw filename?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code has been rewritten many times since i looked at it and i don't recall the reason.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@christoffer I think you can go ahead and remove the stripping of the extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonaskello Hi Jonas. Sorry for the delay, PTOs and what-not came in the way.

Would you be OK if we remove the extension in a separate diff? I feel that it's unrelated to the change I'm introducing here. And since it might have side effects, it would be nice to keep that change separate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My original comment was probably unclear. I was only asking out of curiosity, not as a way to ask if I should remove it now :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@christoffer Sure, doing it in a separate diff makes sense.

);
}

// No field in package json was a valid option. Continue with the next path.
return findFirstExistingPath(
tryPaths,
readJson,
fileExists,
doneCallback,
index + 1,
mainFields
);
}
);
}

// This is async code, we need to return unconditionally, otherwise the code still falls
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonaskello Moved it here, let me know what you thing of the formulation. Changed "return in an else-branch otherwise" -> "return unconditionally, otherwise"

// through and keeps recursing. While this might work in general, libraries that use neo-async
// like Webpack will actually not allow you to call the same callback twice.
//
// An example of where this caused issues:
// https://github.com/dividab/tsconfig-paths-webpack-plugin/issues/11
//
// Continue with the next path
return findFirstExistingPath(
tryPaths,
readJson,
fileExists,
doneCallback,
index + 1,
mainFields
);
});
} else {
TryPath.exhaustiveTypeException(tryPath.type);
Expand Down
49 changes: 23 additions & 26 deletions src/match-path-sync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,23 +79,22 @@ export function matchFromAbsolutePaths(
return findFirstExistingPath(tryPaths, readJson, fileExists, mainFields);
}

/**
* Given a parsed package.json object, get the first field name that is defined
* in a list of prioritized field names to try.
*
* @param packageJson Parsed JSON object from package.json. May be undefined.
* @param mainFields A list of field names to try (in order)
* @returns The first matched field name in packageJson, or undefined.
*/
export function getPrioritizedMainFieldName(
packageJson: Filesystem.PackageJson | undefined,
mainFields: string[]
function findFirstExistingMainFieldMappedFile(
packageJson: Filesystem.PackageJson,
mainFields: string[],
packageJsonPath: string,
fileExists: Filesystem.FileExistsSync
): string | undefined {
if (packageJson) {
for (let index = 0; index < mainFields.length; index++) {
const mainFieldName = mainFields[index];
if (packageJson[mainFieldName]) {
return mainFieldName;
for (let index = 0; index < mainFields.length; index++) {
const mainFieldName = mainFields[index];
const candidateMapping = packageJson[mainFieldName];
if (candidateMapping && typeof candidateMapping === "string") {
const candidateFilePath = path.join(
path.dirname(packageJsonPath),
candidateMapping
);
if (fileExists(candidateFilePath)) {
return candidateFilePath;
}
}
}
Expand All @@ -121,18 +120,16 @@ function findFirstExistingPath(
}
} else if (tryPath.type === "package") {
const packageJson: Filesystem.PackageJson = readJson(tryPath.path);
const mainFieldName = getPrioritizedMainFieldName(
packageJson,
mainFields
);
if (mainFieldName) {
const file = path.join(
path.dirname(tryPath.path),
packageJson[mainFieldName]
if (packageJson) {
const mainFieldMappedFile = findFirstExistingMainFieldMappedFile(
packageJson,
mainFields,
tryPath.path,
fileExists
);
if (fileExists(file)) {
if (mainFieldMappedFile) {
// Not sure why we don't just return the full path? Why strip it?
return Filesystem.removeExtension(file);
return Filesystem.removeExtension(mainFieldMappedFile);
}
}
} else {
Expand Down
72 changes: 68 additions & 4 deletions test/match-path-async-tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -187,18 +187,82 @@ describe("match-path-async", () => {
},
["missing", "browser", "main"]
);
const existingPath = join("/root", "location", "mylib", "christoffer.ts");
const mainFilePath = join("/root", "location", "mylib", "kalle.ts");
const browserFilePath = join("/root", "location", "mylib", "browser.ts");
const existingFiles = [mainFilePath, browserFilePath];

matchPath(
"lib/mylib",
(_path, callback) =>
callback(undefined, {
main: "./kalle.ts",
browser: "./christoffer.ts"
browser: "./browser.ts"
}),
(path, callback) => callback(undefined, path === existingPath),
(path, callback) =>
callback(undefined, existingFiles.indexOf(path) !== -1),
undefined,
(_err, result) => {
assert.equal(result, removeExtension(browserFilePath), result);
done();
}
);
});

it("should not resolve field name mapped file that doesn't exist", done => {
const matchPath = createMatchPathAsync(
"/root/",
{
"lib/*": ["location/*"]
},
["browser", "main"]
);
const existingFile = join("/root", "location", "mylib", "kalle.ts");

matchPath(
"lib/mylib",
(_path, callback) =>
callback(undefined, {
main: "./kalle.ts",
browser: "./missing-file.ts"
}),
(path, callback) => callback(undefined, path === existingFile),
undefined,
(_err, result) => {
assert.equal(result, removeExtension(existingFile), result);
done();
}
);
});

it("should not resolve advanced field name mappings", done => {
const matchPath = createMatchPathAsync(
"/root/",
{
"lib/*": ["location/*"]
},
["browser", "main"]
);
const mainFilePath = join("/root", "location", "mylib", "kalle.ts");
const browserFilePath = join(
"/root",
"location",
"mylib",
"advanced-replacement.ts"
);
const existingFiles = [mainFilePath, browserFilePath];

matchPath(
"lib/mylib",
(_path, callback) =>
callback(undefined, {
main: "./kalle.ts",
browser: { mylib: "./advanced-replacement.ts" }
}),
(path, callback) =>
callback(undefined, existingFiles.indexOf(path) !== -1),
undefined,
(_err, result) => {
assert.equal(result, removeExtension(existingPath), result);
assert.equal(result, removeExtension(mainFilePath), result);
done();
}
);
Expand Down
58 changes: 52 additions & 6 deletions test/match-path-sync-tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,16 +118,16 @@ describe("match-path-sync", () => {

it("should resolve from main field in package.json and correctly remove file extension", () => {
const matchPath = createMatchPath("/root/", { "lib/*": ["location/*"] });
const existingPath = join("/root", "location", "mylibjs", "kallejs");
const existingPath = join("/root", "location", "mylibjs", "kalle.js");
// Make sure we escape the "."
const result = matchPath(
"lib/mylibjs",
(_: string) => ({ main: "./kallejs" }),
(_: string) => ({ main: "./kalle.js" }),
(name: string) => name === existingPath,
[".ts", ".js"]
);

assert.equal(result, existingPath);
assert.equal(result, removeExtension(existingPath));
});

it("should resolve from list of fields by priority in package.json", () => {
Expand All @@ -136,16 +136,62 @@ describe("match-path-sync", () => {
"browser",
"main"
]);
const existingPath = join("/root", "location", "mylibjs", "christofferjs");
const mainFilePath = join("/root", "location", "mylibjs", "main.js");
const browserFilePath = join("/root", "location", "mylibjs", "browser.js");
const existingPaths = [mainFilePath, browserFilePath];

// Make sure we escape the "."
const result = matchPath(
"lib/mylibjs",
(_: string) => ({ main: "./main.js", browser: "./browser.js" }),
(name: string) => existingPaths.indexOf(name) !== -1,
[".ts", ".js"]
);

assert.equal(result, removeExtension(browserFilePath));
});

it("should ignore field mappings to missing files in package.json", () => {
const matchPath = createMatchPath("/root/", { "lib/*": ["location/*"] }, [
"browser",
"main"
]);
const existingPath = join("/root", "location", "mylibjs", "kalle.js");
// Make sure we escape the "."
const result = matchPath(
"lib/mylibjs",
(_: string) => ({ main: "./kallejs", browser: "./christofferjs" }),
(_: string) => ({
main: "./kalle.js",
browser: "./nope.js"
}),
(name: string) => name === existingPath,
[".ts", ".js"]
);

assert.equal(result, existingPath);
assert.equal(result, removeExtension(existingPath));
});

it("should ignore advanced field mappings in package.json", () => {
const matchPath = createMatchPath("/root/", { "lib/*": ["location/*"] }, [
"browser",
"main"
]);
const mainFilePath = join("/root", "location", "mylibjs", "kalle.js");
const browserFilePath = join("/root", "location", "mylibjs", "browser.js");
const existingPaths = [mainFilePath, browserFilePath];

// Make sure we escape the "."
const result = matchPath(
"lib/mylibjs",
(_: string) => ({
main: "./kalle.js",
browser: { mylibjs: "./browser.js", "./kalle.js": "./browser.js" }
}),
(name: string) => existingPaths.indexOf(name) !== -1,
[".ts", ".js"]
);

assert.equal(result, removeExtension(mainFilePath));
});

it("should resolve to with the help of baseUrl when not explicitly set", () => {
Expand Down