dividab / tsconfig-paths

Load node modules according to tsconfig paths, in run-time or via API.
MIT License
1.82k stars 104 forks source link

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

Closed katywings closed 2 years ago

katywings commented 4 years ago
cspotcode commented 4 years ago

@katywings Thanks, I reviewed and ended up with more questions than when I started, so I posted a theory here: https://github.com/dividab/tsconfig-paths/issues/133#issuecomment-663927470

jonaskello commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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

jonaskello commented 4 years ago

@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 commented 4 years ago

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

jonaskello commented 4 years ago

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 commented 4 years ago

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:

https://github.com/dividab/tsconfig-paths/blob/2461cc9c0aa6f02f27c679d1344adaf35ff05cf7/test/data/match-path-data.ts#L115-L126

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 commented 2 years ago

Closed in #193