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.

Sign up for GitHub

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

Jump to bottom

fix: regard file extensions from package.json during path resolution (#133) #135

Closed
wants to merge 2 commits into from
Closed

fix: regard file extensions from package.json during path resolution (#133) #135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d17cea
fix: regard file extensions during path resolution (#133)
katywings Jul 24, 2020
2380f89
test: consider extensions in resolved paths (#133)
katywings Jul 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 0 additions & 4 deletions src/filesystem.ts
View file
Open in desktop
Expand Up @@ -81,7 +81,3 @@ export function fileExistsAsync(
callback2(undefined, stats ? stats.isFile() : false);
});
}

export function removeExtension(path: string): string {
return path.substring(0, path.lastIndexOf(".")) || path;
}
7 changes: 1 addition & 6 deletions src/match-path-async.ts
View file
Open in desktop
Expand Up @@ -148,7 +148,6 @@ function findFirstExistingPath(
return doneCallback(err);
}
if (exists) {
// Not sure why we don't just return the full path? Why strip it?
return doneCallback(undefined, TryPath.getStrippedPath(tryPath));
}
if (index === tryPaths.length - 1) {
Expand Down Expand Up @@ -180,11 +179,7 @@ function findFirstExistingPath(
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)
);
return doneCallback(undefined, mainFieldMappedFile);
}

// No field in package json was a valid option. Continue with the next path.
Expand Down
4 changes: 1 addition & 3 deletions src/match-path-sync.ts
View file
Open in desktop
Expand Up @@ -118,7 +118,6 @@ function findFirstExistingPath(
tryPath.type === "index"
) {
if (fileExists(tryPath.path)) {
// Not sure why we don't just return the full path? Why strip it?
return TryPath.getStrippedPath(tryPath);
}
} else if (tryPath.type === "package") {
Expand All @@ -131,8 +130,7 @@ function findFirstExistingPath(
fileExists
);
if (mainFieldMappedFile) {
// Not sure why we don't just return the full path? Why strip it?
return Filesystem.removeExtension(mainFieldMappedFile);
return mainFieldMappedFile;
}
}
} else {
Expand Down
10 changes: 3 additions & 7 deletions src/try-path.ts
View file
Open in desktop
@@ -1,7 +1,6 @@
import * as path from "path";
import { MappingEntry } from "./mapping-entry";
import { dirname } from "path";
import { removeExtension } from "./filesystem";

export interface TryPath {
readonly type: "file" | "extension" | "index" | "package";
Expand Down Expand Up @@ -60,15 +59,12 @@ export function getPathsToTry(
return pathsToTry.length === 0 ? undefined : pathsToTry;
}

// Not sure why we don't just return the full found path?
export function getStrippedPath(tryPath: TryPath): string {
return tryPath.type === "index"
? dirname(tryPath.path)
: tryPath.type === "file"
? tryPath.path
: tryPath.type === "extension"
? removeExtension(tryPath.path)
: tryPath.type === "package"
: tryPath.type === "file" ||
Copy link
Member

Choose a reason for hiding this comment

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

This change affect other cases than when the file is coming from package.json fields (i.e the extensions case). You can see this by the fact the tests that are not releated to package.json needed changing. This change should probably be done too but in the interest of keeping the changes small I think we can wait with this change to later so this PR will only affect the case where the file is coming directly from package.json fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It probably then makes sense that you directly change this stuff by yourself, this way you can implement each detail how you want it ;).

Copy link
Member

Choose a reason for hiding this comment

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

My concern is that we may be breaking cases that we have not thought about. There are quite a few ppl using this package so the backlash may be quite large :-). So merging this PR will be easier the fewer cases we might be breaking. I'm thinking we can merge and publish this one change of how package.json main files are resolved and see if we get a lot of backlash for doing this. If it works out we can then look at making more changes, for example how extensions are resolved for regular files (not from package.json).

So if you could just revert the changes to getStrippedPath() and the related tests that would be great. I think your case will still be fixed by the remaining changes?

Copy link
Contributor Author

@katywings katywings Jul 26, 2020 •
edited

Choose a reason for hiding this comment

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

I don't have more time to work on this ;). I am behind my own schedule since some time. So if you want more changes, go for it, everything is open source 😅.

My concern is that we may be breaking cases that we have not thought about.

I respect this concern ^^! I have a long-term recommendation for you regarding this topic: too me it seems like this concern comes from the absence of integration tests, this makes it really hard for contributors to implement sensible changes!

Copy link
Member

Choose a reason for hiding this comment

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

The concern actually comes from my very limited time to work on this :-) I don't have the time to handle any backlash so I want to minimize it as far as possible. Making a change is often quick and easy but maintaining the effects of the change can be really time consuming. Integration tests are great but having help maintaining the package is even better. If you don't have the time to work on it I'll leave it open and see if someone else will pick it up.

@cspotcode Perhaps you want to continue the work on this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's a chicken-and-egg problem. Without proper documentation for how this library is supposed to behave, no one will want to help us maintain it.

For example, the createMatchPath docs don't explain how it resolves. This leads me to assume they return absolute paths, but apparently that's not true because extensions get stripped off. And now no one can remember why it does this. If it returned absolute paths, my assumption would be correct, and no docs needed. But the real behavior is strange, so it needs to be explained.

I think you and I will need to make the necessary behavior much clearer to both consumers and contributors.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I agree it is not good that the rules for resolution is not clearer. In my view part of why the clarity is missing is becuase of the stripping of extensions and paths. A problem is that it already works for a lot of ppl so whatever we do could be breaking and we don't know since the rules are not explicitly documented. There are tests but this PR will break the tests. I think we should try do clear it up and release a major version so we can break things.

@cspotcode Do you think we close this PR then and continue work elsewhere?

tryPath.type === "extension" ||
tryPath.type === "package"
? tryPath.path
: exhaustiveTypeException(tryPath.type);
}
Expand Down
40 changes: 19 additions & 21 deletions test/data/match-path-data.ts
View file
Open in desktop
@@ -1,5 +1,4 @@
import { join, dirname } from "path";
import { removeExtension } from "../../src/filesystem";

export interface OneTest {
readonly name: string;
Expand Down Expand Up @@ -60,7 +59,7 @@ export const tests: ReadonlyArray<OneTest> = [
existingFiles: [join("/root", "location", "mylib.myext")],
requestedModule: "lib/mylib",
extensions: [".js", ".myext"],
expectedPath: removeExtension(join("/root", "location", "mylib.myext")),
expectedPath: join("/root", "location", "mylib.myext"),
},
{
name: "should resolve request with extension specified",
Expand All @@ -78,7 +77,7 @@ export const tests: ReadonlyArray<OneTest> = [
},
existingFiles: [join("/root", "location", "foo.ts")],
requestedModule: "lib/foo",
expectedPath: removeExtension(join("/root", "location", "foo.ts")),
expectedPath: join("/root", "location", "foo.ts"),
},
{
name: "should resolve to parent folder when filename is in subfolder",
Expand All @@ -95,9 +94,7 @@ export const tests: ReadonlyArray<OneTest> = [
existingFiles: [join("/root", "location", "mylib", "kalle.ts")],
packageJson: { main: "./kalle.ts" },
requestedModule: "lib/mylib",
expectedPath: removeExtension(
join("/root", "location", "mylib", "kalle.ts")
),
expectedPath: join("/root", "location", "mylib", "kalle.ts"),
},
{
name: "should resolve from main field in package.json (js)",
Expand All @@ -107,9 +104,7 @@ export const tests: ReadonlyArray<OneTest> = [
packageJson: { main: "./kalle.js" },
requestedModule: "lib/mylib.js",
extensions: [".ts", ".js"],
expectedPath: removeExtension(
join("/root", "location", "mylib.js", "kalle.js")
),
expectedPath: join("/root", "location", "mylib.js", "kalle.js"),
},
{
name:
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be a test for preventing what do in this PR. Probably this test was added for a reason but I'm not sure what...

"should resolve from main field in package.json and correctly remove file extension",

Expand All @@ -120,9 +115,7 @@ export const tests: ReadonlyArray<OneTest> = [
packageJson: { main: "./kalle.js" },
extensions: [".ts", ".js"],
requestedModule: "lib/mylibjs",
expectedPath: removeExtension(
join("/root", "location", "mylibjs", "kalle.js")
),
expectedPath: join("/root", "location", "mylibjs", "kalle.js"),
},
{
name: "should resolve from list of fields by priority in package.json",
Expand All @@ -136,9 +129,7 @@ export const tests: ReadonlyArray<OneTest> = [
],
extensions: [".ts", ".js"],
requestedModule: "lib/mylibjs",
expectedPath: removeExtension(
join("/root", "location", "mylibjs", "browser.js")
),
expectedPath: join("/root", "location", "mylibjs", "browser.js"),
},
{
name: "should ignore field mappings to missing files in package.json",
Expand All @@ -152,9 +143,7 @@ export const tests: ReadonlyArray<OneTest> = [
browser: "./nope.js",
},
extensions: [".ts", ".js"],
expectedPath: removeExtension(
join("/root", "location", "mylibjs", "kalle.js")
),
expectedPath: join("/root", "location", "mylibjs", "kalle.js"),
},
{
name: "should ignore advanced field mappings in package.json",
Expand All @@ -170,9 +159,7 @@ export const tests: ReadonlyArray<OneTest> = [
browser: { mylibjs: "./browser.js", "./kalle.js": "./browser.js" },
},
extensions: [".ts", ".js"],
expectedPath: removeExtension(
join("/root", "location", "mylibjs", "kalle.js")
),
expectedPath: join("/root", "location", "mylibjs", "kalle.js"),
},
{
name: "should resolve to with the help of baseUrl when not explicitly set",
Expand Down Expand Up @@ -209,4 +196,15 @@ export const tests: ReadonlyArray<OneTest> = [
requestedModule: "lib/mylib",
expectedPath: undefined,
},
{
name: "should resolve main file with cjs file extension",
absoluteBaseUrl: "/root/",
paths: {},
existingFiles: [join("/root", "mylib", "index.cjs")],
packageJson: {
main: "./index.cjs",
},
requestedModule: "mylib",
expectedPath: join("/root", "mylib", "index.cjs"),
},
];