Add path mapping support to ESM and CJS loaders

[geigerzaehler]

trafficstars

The ESM loader now resolves import paths using TypeScripts path mapping feature.

EDIT: Closes #1586

[cspotcode]

Thanks for this. Because of the American holidays, I have somewhat of a backlog of issues and PRs which need attention before I'll be able to properly review this. There may be some delay.

I see some refactoring in node-errors; can you explain the motivation for some of those changes? It's good to have an explanation for such changes when reviewing.

Your description says this enables path mapping for the ESM loader. We'd like to also extend the same functionality to the CommonJS loader. I see that your createPathMapper implementation looks clean and modular; do you anticipate any issues if we attempt to use it for CommonJS as well? We'd hook into Module._resolveFilename for this.

Is this feature-flagged? We should decide if it's enabled or disabled by default, and decide how users can opt-in or opt-out.

[geigerzaehler]

There may be some delay.

No problem.

I see some refactoring in node-errors; can you explain the motivation for some of those changes?

The new resolving code needs to detect ERR_MODULE_NOT_FOUND errors so it can try more mapped candidates. The builtin NodeError from Node’s internal error module has a code property that allows you to distinguish errors. The change in node-errors also sets this property on the error instances. The new logic for defining errors is similar to the code in Node’s error module.

do you anticipate any issues if we attempt to use it for CommonJS as well?

I don’t think there will be any issues. The path mapping code is does not include any logic that is specific to ESM resolution. The only inputs to it are the TypeScript compiler options.

Is this feature-flagged? We should decide if it's enabled or disabled by default, and decide how users can opt-in or opt-out.

It’s not feature-flagged at the moment because I couldn’t think of a scenario where you would want the path mapping to be disabled. If you have configured the paths compiler option and you are using it in your code, your code will only run if the path mapping is enabled. If path mapping is disabled the code will always fail at the module resolution stage which makes the option unusable. So I’d lean towards not feature-flagging this for simplicity. But if there’s a good reason to put this behind a feature flag it should be easy to accomplish

[cspotcode]

If you have configured the paths compiler option and you are using it in your code, your code will only run if the path mapping is enabled.

Good point. And I think someone could disable it like this:

{
  "compilerOptions": {
    "paths": { ... }
  },
  "ts-node": {
    "compilerOptions": {
      "paths": {} // empty object: disable path mappings in ts-node
    }
  }
}

[codecov[bot]]

Codecov Report

Merging #1585 (6632481) into main (ff78b64) will increase coverage by 1.24%. The diff coverage is 93.49%.

Impacted Files	Coverage Δ
src/configuration.ts	86.06% <ø> (ø)
src/index.ts	79.56% <71.42%> (-0.13%)	:arrow_down:
src/esm.ts	84.72% <92.30%> (+1.38%)	:arrow_up:
src/path-mapping.ts	92.85% <92.85%> (ø)
src/cjs-resolve-hooks.ts	87.17% <96.00%> (+13.84%)	:arrow_up:
dist-raw/node-internal-errors.js	97.50% <100.00%> (+1.84%)	:arrow_up:
dist-raw/node-internal-modules-cjs-loader.js	76.42% <0.00%> (+4.56%)	:arrow_up:
dist-raw/node-internalBinding-fs.js	100.00% <0.00%> (+7.69%)	:arrow_up:

[geigerzaehler]

I definitely want to add CommonJS support which shouldn't be too difficult. I'm already familiar with the hooking API.

I’d prefer to keep scope of this PR small and I’m not familiar enough with the CommonJS side of things so I’ll leave this to you.

One thing to consider is that we’d probably want the users to explicitly opt in to path mapping for CommonJS. They’ll be using tsconfig-paths at the moment and it is unclear if the ts-node implementation can be used side-by-side or compatible.

[cspotcode]

Fair point. Would it be a great hardship for those users to disable tsconfig-paths? I hope they would appreciate the simplicity of removing a component. I would like our default behaviour to be the best forward-thinking behaviour, but perhaps we add an optional flag which can be used to selectively disable it for commonjs? Or we somehow detect tsconfig-paths and throw a warning when both mappers are competing?

On Mon, Jan 24, 2022 at 6:15 AM Thomas Scholtes @.***> wrote:

I definitely want to add CommonJS support which shouldn't be too difficult. I'm already familiar with the hooking API.

I’d prefer to keep scope of this PR small and I’m not familiar enough with the CommonJS side of things so I’ll leave this to you.

One thing to consider is that we’d probably want the users to explicitly opt in to path mapping for CommonJS. They’ll be using tsconfig-paths at the moment and it is unclear if the ts-node implementation can be used side-by-side or compatible.

— Reply to this email directly, view it on GitHub https://github.com/TypeStrong/ts-node/pull/1585#issuecomment-1019987638, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC35OGJBEFQKHZNAXQ5F33UXUYERANCNFSM5K6CBQ2Q . You are receiving this because you commented.Message ID: @.***>

[cspotcode]

I made an attempt at adding the CommonJS resolver hook. Still some outstanding TODOs that I need to address, but this is looking good.

[cspotcode]

Looks like we have test failures on Windows due to module specifiers using forward slashes and windows paths using backslashes. I haven't checked the code yet, but we probably have some logic assuming a module specifier and a path will always look the same. Should be an easy fix.

https://github.com/TypeStrong/ts-node/runs/4932463748?check_suite_focus=true#step:15:167

[geigerzaehler]

Would it be a great hardship for those users to disable tsconfig-paths?

I don’t think it would. I just wanted to point out, that this is a breaking change. I do believe most users will appreciate this replacement.

Considering the release strategy it probably makes sense to have the commonjs mapping as opt-in and ask users to enable it to collect feedback. Once the feature is stable it can be enabled by default (or always) in a new major release.

Or we somehow detect tsconfig-paths and throw a warning when both mappers are competing?

This sounds complex and error-prone. I think I’d prefer a solution where it is explicitly configured

[cspotcode]

I like that plan: opt-in via a flag today, enable by default in the next major version, get feedback in the meantime.

How about experimentalPathMapping: 'both' | 'cjs' | 'esm' | 'none'? Where 'esm' is the default today, and 'both' is the default in the next major version, with the flag renamed to pathMapping?

[cspotcode]

I see that tsconfig-paths does some special-casing for node's built-in modules. I'm not sure if we need to do the same. https://github.com/dividab/tsconfig-paths/blob/0b259d4cf6cffbc03ad362cfc6bb129d040375b7/src/register.ts#L80-L88

Given the following admittedly contrived tsconfig;

"paths": {
  "*": ["src/*", "*"]

How should we resolve:

import fs from 'fs';
import lodash from 'lodash';

I'll do some testing with tsc --traceResolutions

[cspotcode]

What if someone is using paths to declare types via .d.ts files? For example they want import "colors" to resolve to src/types/colors.d.ts but they want node to execute colors from node_modules?

[cspotcode]

I think we'll need to add a fallback so that when mappings fail, the unmapped specifier is resolved, too. This matches tsc's behavior.

[cspotcode]

We also need to handle when baseUrl is set and paths is not. It appears to be equivalent to paths: {"*": ["*"]}

EDIT: is this "*": ["*"] always implied, even when the user specifies a paths object?

[geigerzaehler]

How about experimentalPathMapping: 'both' | 'cjs' | 'esm' | 'none'? Where 'esm' is the default today, and 'both' is the default in the next major version, with the flag renamed to pathMapping?

I like it!

I see that tsconfig-paths does some special-casing for node's built-in modules. I'm not sure if we need to do the same.

In my opinion it’s preferable to keep the mapping algorithm as simple as possible. This means less surprises for the user and makes fewer assumptions on how ts-node is used. Considering the ability to compose multiple ESM loaders means that any special cases may interfere with other loaders and have unintended effects.

In the particular case of preserving builtin node modules I’m not sure that even makes sense. If we remap fs, for example, the type checker will use the mapped file for type checking. So ts-node should also run the mapped file.

What if someone is using paths to declare types via .d.ts files? For example they want import "colors" to resolve to src/types/colors.d.ts but they want node to execute colors from node_modules?

That’s an interesting case. There are other ways to achieve this, for example the typeRoots setting and type references. Using the same rationale (simplicity and predictability) as above I’d argue that users use one of the alternatives instead of baking this into the mapping algorithm.

We also need to handle when baseUrl is set and paths is not. It appears to be equivalent to paths: {"*": ["*"]}

EDIT: is this "*": ["*"] always implied, even when the user specifies a paths object?

Wow. Great find. I tested the last question and the mapping * -> * is not implied if the paths object is set.

[cspotcode]

I found why tsconfig-paths prefers node's builtins: https://github.com/dividab/tsconfig-paths/pull/60

I also did some tests.

• the "*": ["*"] mapping is implied when the paths object is omitted or when the paths object has no "*" key
• @types/node builtin declarations are preferred over mapped modules. This matches tsconfig-paths' preference for node builtins

Here are the tests: https://github.com/TypeStrong/ts-node-repros/tree/ts-path-mapping-tests Let me know if you get different results

[cspotcode]

I've merged #1614 into this PR, meant to establish a common core for _resolveFilename hooks.

The findings from https://github.com/TypeStrong/ts-node/pull/1585#issuecomment-1022241341 still need to be addressed.

[geigerzaehler]

• @types/node builtin declarations are preferred over mapped modules. This matches tsconfig-paths' preference for node builtins

This behavior extends to all modules with ambient declarations. For example

// ambient.d.ts
declare module "foo" {
  export {}
}

// index.d.ts
/// <reference path="./ambient.d.ts" />
import foo from "foo"
foo()

// src/foo.ts
export default function () {}

Even if foo is mapped to src/foo.ts the type checker uses the declaration of module foo in ambient.d.ts.

If we don’t remap node modules as a special, case we create inconsistent behavior.

I think ideally we want a behavior where the modules used at runtime are the same as the modules resolved at type check-time. This is impossible because we don’t know where the module code for a ambient declaration is by the nature of the declaration. I’d also consider the case where path is resolved to a module that has a ambient declaration otherwise as uncommon.

With this in mind I’d suggest not treat remapping uniformly and not do any special casing.

[cspotcode]

I think we still need to avoid this bug? https://github.com/dividab/tsconfig-paths/issues/56

[geigerzaehler]

I think we still need to avoid this bug? dividab/tsconfig-paths#56

I‘m unsure if I’d consider the issue a bug. The issue boils down to: querystring references the core module when not transpiled and references the installed module when transpiled with the path mapping * -> node_modules/*. This seems like expected behavior to me.

We should also keep in mind that with ESM imports we have more workarounds for these kind of edge cases. We can always explicitly refrence a node core module with node:querystring, for example.

[cspotcode]

I‘m unsure if I’d consider the issue a bug. The issue boils down to: querystring references the core module when not transpiled and references the installed module when transpiled with the path mapping * -> node_modules/*. This seems like expected behavior to me.

I see what you're saying. And there should be no need for the node_modules/* mapping. If path mapping fails, then it should fallback to node's resolver which should look at node built-ins first, then node_modules.

Users hit performance issues with this: https://github.com/dividab/tsconfig-paths/issues/56#issuecomment-416596613 https://github.com/dividab/tsconfig-paths/pull/60#issue-355280581

Additionally this increases performance by avoiding unnecessary disk access. For example webpack calls require("crypto") in a loop.

However, if we scope the path mapping so that it only applies to the project's own code, not to node_modules dependencies, then this should be mitigated. If node_modules/foo/index.js does import "express" then we don't want that redirecting to ./src/express.ts Or if webpack calls require('crypto') in a loop, our path mappings should not apply.

I think the logic's already there, but we'll need tests to cover these cases.

• test that path mapping does not affect require calls coming from node_modules/*/*
• test that we fallback to node's resolver when path mapping fails (if ''->'src/' and import "lodash", should fallback to finding node_modules/lodash)

[cspotcode]

Any update on this? Any thoughts on the proposed test coverage? I'm wondering if this will make it in for v10.6.0

[charles-allen]

Any update on this? Any thoughts on the proposed test coverage? I'm wondering if this will make it in for v10.6.0

Are you still looking for someone to add some tests? I have some time today :)

[cspotcode]

Yeah, that'd be great. I made a list in a comment, I think if we can get those tests added -- even if they fail initially -- that'll be a huge step forward. Don't hesitate to ask for clarification on anything.

[cspotcode]

@charles-allen any luck? Anything I can help with? I'm always looking for excuses to improve CONTRIBUTING.md

[charles-allen]

@charles-allen any luck? Anything I can help with? I'm always looking for excuses to improve CONTRIBUTING.md

I forked the PR, cloned, ran the tests (3 are failing already?), and started looking at the code. That was all pretty straight-forward; no issues so far. I can see some new tests in the diff -- I assume the tests I need to write will be similar to those. Mostly I just need to squeeze out a little more time; I really want to get it done this week, with sooner === better!

I'll try to get one test written & pushed tomorrow so I can share my repo URL here for early feedback

[cspotcode]

Sounds great, thanks. I listed some of the tests we need here: https://github.com/TypeStrong/ts-node/pull/1585#pullrequestreview-869432652 (I forget if I already linked to this comment or not)

ran the tests (3 are failing already?)

Yeah, I forget exactly why, but that is expected. The new tests will also fail, but once we have them, it'll be that much easier to finish the implementation.

I can see some new tests in the diff -- I assume the tests I need to write will be similar to those.

Yeah, this style of test seems good for simplicity, since each is basically its own, mini-project.

[charles-allen]

Quick update -- I didn't do much Wednesday; but I have blocked the whole day tomorrow to work on this :)

[charles-allen]

@cspotcode - I've pushed an attempt at the first 4 tests to my fork: https://github.com/charles-allen-forks/ts-node/tree/path-mapping https://github.com/charles-allen-forks/ts-node/commits/path-mapping

So far I have 2 questions:

• I think exec is not working correctly in my tests, but I can't work out what I've done differently to the 2 existing ESM tests. Any ideas?
• You listed ~7 tests; but I should replicate these for both ESM & CJS cases, right? So 14 tests total?

I plan to continue tomorrow or Sunday -- still hoping to wrap up the tests this week!

[cspotcode]

@charles-allen

I think exec is not working correctly in my tests, but I can't work out what I've done differently to the 2 existing ESM tests. Any ideas?

Hmm, I'm not sure. What's going wrong?

You listed ~7 tests; but I should replicate these for both ESM & CJS cases, right? So 14 tests total?

Ideally yeah. But if you want, and you think it makes sense, I'm open to use some clever ways to automate the ESM & CJS testing so that you can use one set of test files to test both permutations.