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

[katywings]

• Related to the comment from @jonaskello baseUrl has a side effect on the used dependency export #133 (comment)
• Adds support for cjs main entries by considering file extensions during path resolution
  • I added an extra test for the cjs main entry
  • Also tried out the new build with my original colorette cjs error reproduction zip from baseUrl has a side effect on the used dependency export #133

[cspotcode]

@katywings Thanks, I reviewed and ended up with more questions than when I started, so I posted a theory here: #133 (comment)

[jonaskello]

@katywings @cspotcode Thanks for working on this :-)

I'll review further but before I forget I just wanted to note that we also need to fix the async version. It is a bit unfortunate but there are two versions of some of the exported API functions, one for sync and one for async. IIRC the async ones were added to support the webpack plugin while the sync versions were kept for compability.

So whenever a change is made to this file:
https://github.com/dividab/tsconfig-paths/blob/master/src/match-path-sync.ts
An analog change must also be made to this file:
https://github.com/dividab/tsconfig-paths/blob/master/src/match-path-async.ts

IIRC the design idea was that the core logic is kept as pure functions in this file (so this file should only have pure functions):
https://github.com/dividab/tsconfig-paths/blob/master/src/try-path.ts

While the side-effect parts are in sync/async files mentioned above.

[jonaskello]

@katywings It seems some changes in this PR is just formatting (or perhaps I am missing something). It should not really be possible to get inconsistent formatting since we are using husky with lint-staged and prettier :-). Not sure why this happens. I upgraded prettier and related packages on master. Perhaps you can merge master into this PR (or rebase it)?

EDIT: You can also try re-formatting all files with prettier manually with yarn prettier --write .

[katywings]

@jonaskello You literally changed the prettier config in master 17 minutes ago c4ca84c... Of course the formatting of this pr is different then. I gonna update it.

[jonaskello]

@katywings Yes, but the formatting was different before too. In fact the discrepancy in formatting was the only reason I made the change you mention :-).

[katywings]

@jonaskello Here ya go, the branch is rebased ^^

[jonaskello]

Thanks, I see now that there was actually changes on those lines I thought was formatting :-). Btw the reason you see changes in formatting coming from my upgrade of prettier is that I decided to go with prettier's new defaults when upgrading (I first tried to retain the old formatting by fiddling with the config but then decided against it and removed the config again).

[jonaskello]

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",

[jonaskello]

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?

[katywings]

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

[jonaskello]

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?

[katywings]

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!

[jonaskello]

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?

[cspotcode]

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.

[jonaskello]

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?

[jonaskello]

The scope of this PR will be to use filenames found in main fields in package.json as-is without removing the extension etc.

To recap the problem it will solve is this:

The colorette package has this is it's package.json:

{
  "name": "colorette",
  "version": "1.2.1",
  "main": "index.cjs",
  "type": "module",
  "module": "index.js",
  "exports": {
    "./package.json": "./package.json",
    ".": {
      "require": "./index.cjs",
      "import": "./index.js"
    }
  },
}

This makes the package compatbile with both old and new node resolution. The old versions of node only use requireand only look a the main field and thus resolve to index.cjs which is a CommonJS module so it works. Newer versions of node looks at the type field and a value of module means that every .js file should be considered an ECMAScript module (ESM). It will resolve to index.js and treat it as an ESM so it works.

Currently tsconfig-paths only works with old node resolution. So it will only look at the main field and find index.cjs. However it then removes the extension .cjs and passes just index to node. The old version of node will add .js extension and try requrie with index.js. This file is however an ESM and you cannot use require on it so you will get an error.

The solution to this that this PR does it to not remove the extension, so it passes index.cjs to node during require and it works.

There were tests in place to avoid the behavior that this PR will implement, specifically this test:

tsconfig-paths/test/data/match-path-data.ts

Lines 115 to 126 in 2461cc9

	name:
	"should resolve from main field in package.json and correctly remove file extension",
	absoluteBaseUrl: "/root/",
	paths: { "lib/*": ["location/*"] },
	existingFiles: [join("/root", "location", "mylibjs", "kalle.js")],
	packageJson: { main: "./kalle.js" },
	extensions: [".ts", ".js"],
	requestedModule: "lib/mylibjs",
	expectedPath: removeExtension(
	join("/root", "location", "mylibjs", "kalle.js")
	),
	},

I'm concerned about breaking other cases than the one we are trying to fix but I have no concrete cases that I can think of right now that will break.

[jonaskello]

Closed in #193