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

Add path mapping support to ESM and CJS loaders #1585

Open
wants to merge 79 commits into
base: main
Choose a base branch
from

Conversation

geigerzaehler
Copy link

@geigerzaehler geigerzaehler commented Dec 29, 2021

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

EDIT:
Closes #1586

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

[1]: https://www.typescriptlang.org/docs/handbook/module-resolution.html#path-mapping

Signed-off-by: Thomas Scholtes <geigerzaehler@axiom.fm>
@cspotcode
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link

codecov bot commented Dec 29, 2021

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%) ⬇️
src/esm.ts 84.72% <92.30%> (+1.38%) ⬆️
src/path-mapping.ts 92.85% <92.85%> (ø)
src/cjs-resolve-hooks.ts 87.17% <96.00%> (+13.84%) ⬆️
dist-raw/node-internal-errors.js 97.50% <100.00%> (+1.84%) ⬆️
dist-raw/node-internal-modules-cjs-loader.js 76.42% <0.00%> (+4.56%) ⬆️
dist-raw/node-internalBinding-fs.js 100.00% <0.00%> (+7.69%) ⬆️

@cspotcode cspotcode added this to the next milestone Jan 21, 2022
Copy link
Collaborator

@cspotcode cspotcode left a comment

Choose a reason for hiding this comment

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

I left comments from an initial code review. I plan to implement the requested changes myself, but if you are feeling enthusiastic and would like to do them before me, I will certainly appreciate the assistance. No pressure though.

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

src/esm.ts Outdated Show resolved Hide resolved
src/esm.ts Outdated Show resolved Hide resolved
src/esm.ts Outdated Show resolved Hide resolved
src/esm.ts Outdated Show resolved Hide resolved
src/esm.ts Outdated Show resolved Hide resolved
src/esm.ts Outdated Show resolved Hide resolved
): PathMapper {
if (compilerOptions.paths) {
if (!compilerOptions.baseUrl) {
throw new Error(`Compiler option 'baseUrl' required when 'paths' is set`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is compilerOptions.baseUrl guaranteed to be an absolute path, or do we need to resolve it relative to the tsconfig's location?

Copy link
Author

Choose a reason for hiding this comment

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

Good point. I assumed that we always get the configuration from ts.readConfigFile() in src/configuration.ts and the function resolves the base URL relative to the config file. But there are probably other ways to set the base URL like through the command line, API, or environment.

To address this I think it make sense to resolve any base URL that is not a URL but a relative path to the current directory.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, we almost always get the configuration from ts.readConfigFile(), so we should be good. I think the only way to set it otherwise is via the API? Since we don't support all of tsc's command-line flags.

Maybe the correct approach is to resolve baseUrl once within create() so that we have a reliable, absolute baseUrl to be used elsewhere.

Copy link
Author

Choose a reason for hiding this comment

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

Maybe the correct approach is to resolve baseUrl once within create() so that we have a reliable, absolute baseUrl to be used elsewhere.

Alternatively, we could require baseUrl to be absolute and throw an error otherwise. This would allow us to avoid implicit, environment-dependent behavior for users of the API. This might prevent some confusion when path mapping doesn’t work as expected but only fails at the resolution stage instead of the construction stage. (For example a user may think the base URL they set on the API is considered relative to tsconfig.json and their scripts work whenever they are run from the project root. But then they run the script from a subdirectory and it fails.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good, let's do that. In create() we throw an error if baseUrl is not absolute.

Copy link
Collaborator

@cspotcode cspotcode Jan 25, 2022

Choose a reason for hiding this comment

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

Actually, scratch that, seems we always pass options through TypeScript's API to be normalized, even when provided via our API:

$ node
> require('ts-node').create({compilerOptions: {baseUrl: "foobar"}}).config.options.baseUrl
'/home/ubuntu/dev/ts-node/ts-node/path-mapping/foobar'

So I guess we're all good.

tests/esm-path-mapping/index.ts Outdated Show resolved Hide resolved
cspotcode and others added 4 commits January 24, 2022 00:31
Signed-off-by: Thomas Scholtes <geigerzaehler@axiom.fm>
Signed-off-by: Thomas Scholtes <geigerzaehler@axiom.fm>
Signed-off-by: Thomas Scholtes <geigerzaehler@axiom.fm>
@geigerzaehler
Copy link
Author

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
Copy link
Collaborator

cspotcode commented Jan 24, 2022 via email

src/esm.ts Outdated Show resolved Hide resolved
src/esm.ts Show resolved Hide resolved
src/esm.ts Outdated Show resolved Hide resolved
src/esm.ts Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
@cspotcode
Copy link
Collaborator

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
Copy link
Collaborator

cspotcode commented Jan 25, 2022

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
Copy link
Author

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 cspotcode mentioned this pull request Jan 25, 2022
24 tasks
@cspotcode
Copy link
Collaborator

cspotcode commented Jan 25, 2022

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?

@rhummelmose
Copy link

rhummelmose commented Sep 7, 2022

Really need this, please prioritize if possible :)

@menosprezzi
Copy link

@cspotcode How can I help here?

@hpx7
Copy link

hpx7 commented Dec 4, 2022

Is this work still planned?

@RedStars071
Copy link

some news of this pr?

@testgitdl
Copy link

Hello! Do you know when this will be available? Thanks!

@kuksik
Copy link

kuksik commented Mar 16, 2023

some news about this PR?

@mprinc
Copy link

mprinc commented Mar 16, 2023

Sad that the ts-node tool is too complex (including its dependencies) that it is not possible to handle it promptly enough but it rather takes year and half ... honestly the whole ESM model with ts-node + ts paths is broken because of that, and a lot of libs are now delivering ONLY ESM packages ... Thanks God, I am using that workaround (--experimental-specifier-resolution=node --loader ) and more less I am ok, but it is insane that it cannot be integrated properly and that I have to have that ugly scripts:

node --experimental-specifier-resolution=node --loader $COLABO/ts-esm-loader-with-tsconfig-paths.js index.ts rather than just ts-node index.ts ...

@0x80
Copy link

0x80 commented Mar 17, 2023

@mprinc The workaround is not yet clear to me from skimming through this discussion. Could you share the content of your ts-esm-loader-with-tsconfig-paths.js please?

@trim21
Copy link

trim21 commented Mar 17, 2023

suggesting tsx (and also esm/cjs loader in its description) for this feature

@Naddiseo
Copy link

@0x80

@mprinc The workaround is not yet clear to me from skimming through this discussion. Could you share the content of your ts-esm-loader-with-tsconfig-paths.js please?

This is what I'm using with node 16

/*
loader.js

Usage (I'm also using dotenv, but you can omit the dotenv parts if needed):
DOTENV_CONFIG_PATH=.env node -r dotenv/config --loader=./loader.js /bin/path/to/cli.ts
*/
import { pathToFileURL } from "node:url";
import { getFormat, load, resolve as resolveTs, transformSource } from "ts-node/esm";
import * as tsConfigPaths from "tsconfig-paths";

export { getFormat, transformSource, load };

const { absoluteBaseUrl, paths } = tsConfigPaths.loadConfig();
const matchPath = tsConfigPaths.createMatchPath(absoluteBaseUrl, paths);

export function resolve(specifier, context, defaultResolver) {
  const mappedSpecifier = matchPath(specifier);
  if (mappedSpecifier) {
    specifier = `${mappedSpecifier}.js`;

    /*
    the resolve functionality can only work with file URLs, so we need to convert, this is especially
    the case on windows, where the path might not be a valid URL.
    */
    const url = specifier.startsWith("file:") ? specifier : pathToFileURL(specifier.toString());

    return resolveTs(url.toString(), context, defaultResolver);
  } else {
    // If we can't find a mapping, just pass it on to the default resolver
    return resolveTs(specifier, context, defaultResolver);
  }
}

@gudh
Copy link

gudh commented Mar 21, 2023

@trim21 thanks, tsx works like a charm, no configuration needed!

@troywweber7
Copy link

@trim21 I might be missing something... Why tsx and not mts (similar to mjs)? Or maybe I'm confusing who this was directed at...

@trim21
Copy link

trim21 commented Mar 21, 2023

@trim21 I might be missing something... Why tsx and not mts (similar to mjs)? Or maybe I'm confusing who this was directed at...

that's a link, click it

@troywweber7
Copy link

@trim21 thanks. My client wasn't rendering the link. Boo. Will look in browser.

@mprinc
Copy link

mprinc commented Mar 22, 2023

Hi Thijs (@0x80 ) here is the code I am using ts-esm-loader-with-tsconfig-paths.js:

// https://github.com/TypeStrong/ts-node/discussions/1450#discussioncomment-1806115

// "serve": "cross-env TS_NODE_PROJECT=\"tsconfig.build.json\" node --experimental-specifier-resolution=node --loader ./loader.js src/index.ts",
// TS_NODE_PROJECT=\"tsconfig.build.json\" node --experimental-specifier-resolution=node --loader ./loader.js src/index.ts"

import { resolve as resolveTs } from "ts-node/esm";
import * as tsConfigPaths from "tsconfig-paths";
import { pathToFileURL } from "url";

const { absoluteBaseUrl, paths } = tsConfigPaths.loadConfig();
const matchPath = tsConfigPaths.createMatchPath(absoluteBaseUrl, paths);

export function resolve(specifier, ctx, defaultResolve) {
	const match = matchPath(specifier);
	return match ? resolveTs(pathToFileURL(`${match}`).href, ctx, defaultResolve) : resolveTs(specifier, ctx, defaultResolve);
}

export { load, getFormat, transformSource } from 'ts-node/esm'

together with the command again:

# $COLABO is a path to where the file is tored
node  --experimental-specifier-resolution=node --loader $COLABO/ts-esm-loader-with-tsconfig-paths.js index.ts

@mprinc
Copy link

mprinc commented Mar 22, 2023

NOTE: I am investigating tsx and considering already using esBuild actively in my other projects like codemod tools, which I have a great experience with both efficiency and community, I might soon switch to it. It seems pretty straightforward to do it.

@loynoir
Copy link

loynoir commented Apr 24, 2023

suggesting tsx (and also esm/cjs loader in its description) for this feature

@trim21 Coming from privatenumber/tsx#113 😞


Any news about this PR?

Comment on lines +15 to +43
exports.codes = {};

function defineError(code, buildMessage) {
if (!buildMessage) {
buildMessage = (...args) => args.join(' ');
}
}

function createErrorCtor(errorMessageCreator) {
return class CustomError extends Error {
exports.codes[code] = class CustomError extends Error {
constructor(...args) {
super(errorMessageCreator(...args))
super(`${code}: ${buildMessage(...args)}`);
this.code = code;
}
}
}

defineError("ERR_INPUT_TYPE_NOT_ALLOWED");
defineError("ERR_INVALID_ARG_VALUE");
defineError("ERR_INVALID_MODULE_SPECIFIER");
defineError("ERR_INVALID_PACKAGE_CONFIG");
defineError("ERR_INVALID_PACKAGE_TARGET");
defineError("ERR_MANIFEST_DEPENDENCY_MISSING");
defineError("ERR_MODULE_NOT_FOUND", (path, base, type = 'package') => {
return `Cannot find ${type} '${path}' imported from ${base}`;
});
defineError("ERR_PACKAGE_IMPORT_NOT_DEFINED");
defineError("ERR_PACKAGE_PATH_NOT_EXPORTED");
defineError("ERR_UNSUPPORTED_DIR_IMPORT");
defineError("ERR_UNSUPPORTED_ESM_URL_SCHEME");
defineError("ERR_UNKNOWN_FILE_EXTENSION");

Choose a reason for hiding this comment

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

Why is this portion of the file exactly identical to dist-raw/node-errors.js?

@Baterka
Copy link

Baterka commented Jan 4, 2024

Any movement on this?

@sthasam2
Copy link

sthasam2 commented Jan 5, 2024

Is this still in the works?

@Dudeprogram
Copy link

Dudeprogram commented Jan 5, 2024 via email

@billomore
Copy link

tsx doesn't play well with monorepos, namely with decorators in monorepos. swc doesn't play nice with ESM either, not sure if planned to fix. ts-node was the last citadel that worked out of the box until recently, but without any change or update it just stopped... sad :(

@genesiscz
Copy link

we really need this :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set correct code on errors returned by ESM resolver