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

Rewrite imports with ".ts" extension to ".js" to support Nodejs's "--experimental-strip-types" #59597

Closed
3 of 6 tasks
alshdavid opened this issue Aug 11, 2024 · 3 comments · Fixed by #59767
Closed
3 of 6 tasks
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@alshdavid
Copy link

alshdavid commented Aug 11, 2024

🔍 Search Terms

#55346

✅ Viability Checklist

⭐ Context

Nodejs is shipping built-in TypeScript support for local & development projects, where distributing for production still need to compile typescript sources to javascript for them to work.

This can be used in version >=22.6.0 behind the --experimental-strip-types flag.

A point of contention is that the Nodejs TypeScript transpiler relies on importing TypeScript sources using the .ts file extension.

Example;

// node --experimental-strip-types ./index.ts
// index.ts
import * as foo from './foo.ts'

While we can use the new Nodejs flag to run local TypeScript sources during development with .ts file extensions and tsconfig.json#compilerOptions.allowImportingTsExtensions: true; The TypeScript compiler is no longer capable of producing a valid output to run in Nodejs using this source code.

⭐ Discussion

In previous issues, members of the TypeScript team have stated that rewriting imports is out of scope for TypeScript and will not be implemented.

Has this stance changed in light of the newly added TypeScript support in Nodejs?

I am aware that the support status is experimental so Nodejs will make changes before it rolls out generally but it looks like this particular decision is not likely to change

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Aug 12, 2024

Last time we talked about this in the context of "surely relative paths would be fine to rewrite", we got stuck on what to do about dynamic imports. Let's say you write

import staticImport from "./foo.ts";
const staticDynamic = await import("./foo.ts");
const s = atob("Li9mb28udHM="); // decodes to "./foo.ts"
const dynamicDynamic = await import(s);

The idea is that we unambiguously rewrite static imports:

import staticImport from "./foo.ts";
// to
import staticImport from "./foo.js";

When looking at this, let's call it a "static dynamic" import because the argument can be statically analyzed, it seems inconsistent to not also rewrite this

const staticDynamic = await import("./foo.ts");
// to
const staticDynamic = await import("./foo.js");

This seems defensible but is also a little shady; while static ESM imports are known to have certain semantics, we can't really say for sure what's going on with await import("./foo.ts") - you may have used node:module#register to change the loading semantics once your program is running.

When looking at this, let's call it a "dynamic dynamic" import because its argument cannot be statically analyzed, it seems inconsistent to not treat await import(expr) the same as await import("./something")

const dynamicDynamic = await import(s);

But what does that mean? Insert a runtime shim?

const dynamicDynamic = await import(__fixExtension(s));

function __fixExtension(s) {
  // TODO: Make this perfect
  const g = /(\..+)\.ts/.exec(s);
  return g === null ? s : (g[1] + ".js");
}

At that point we're basically inventing our own module system that goes between us and some other layer, plus the prior concerns about the correctness of changing the runtime semantics of await import (this is a blatant design goal violation but I think this is implicit in crossing the rewrite bridge).

So it comes down to:

  • Option 1: Make dynamic and static imports inconsistent
  • Option 2: Make "static dynamic" and "dynamic dynamic" imports inconsistent
  • Option 3: Runtime-shim dynamic import with somewhat questionable semantics

None of those are super appealing and it'd be great to have an ironclad argument for one of them (or some fourth option).

@alshdavid
Copy link
Author

Thanks for the explanation, that's makes sense. SWC has a rewrite import extensions plugin. It bails out on dynamic imports - which makes sense given the above explanation.

This might be a question for the Nodejs team, but guidance on how to write valid TypeScript that can be executed in Nodejs would be great.

@andrewbranch
Copy link
Member

andrewbranch commented Aug 14, 2024

A few of us on the team discussed this in some detail yesterday; here’s a summary of where we are and what design questions need to be answered.

Overview

We agreed that, under this hypothetical flag, a module specifier in an import declaration would be rewritten during JS emit if it meets all of these criteria:

  • it begins with ./ or ../
  • it does not end in a declaration file name (.d.ts, .d.mts, .d.cts, .d.*.ts)
  • it ends in .ts, .tsx, .mts, or .cts

.tsx becomes .jsx under --jsx preserve, .js otherwise. The other TypeScript file extensions always become their JavaScript equivalent (replace t with j).

Critically, checking these criteria and determining the new specifier can be done trivially, using only the input module specifier and compiler options. It does not depend on knowing what other files do or don’t exist in the compilation or on the file system.

Import calls

Ryan pointed out the issues with import calls. It seemed like we were leaning toward supporting extension substitution in them somehow, but we need to decide between the options Ryan laid out above. Nobody had strong objections to the runtime shim, but we agreed there should be some way to opt out of it (probably a separate tsconfig option). There was also some discussion about inlining the shim vs. including it in tslib. (I didn’t fully grok the argument of why it shouldn’t be included in tslib, so maybe @RyanCavanaugh can fill that in if it’s important to bring up here.)

Error checking

While the rewriting procedure itself cannot depend on non-local information, we can and should use module resolution information to issue errors when a module specifier would be rewritten unsafely. (This is analogous to --isolatedModules, which issues errors on patterns that cannot be safely transpiled without non-local information.) This occurs only in CommonJS-style module resolution. For example, the import

import foo = require("./foo.ts");

meets the criteria to be rewritten to "./foo.js". But because this is a require, foo.ts might refer to a directory with a package.json or an index.ts, or even a file named foo.ts.ts. Unless the actual resolved module is a file in the same directory named foo.ts, require("./foo.js") will fail at runtime. We agreed that we should issue an error in these cases.

There is another category of potential runtime error that is less clear whether we can reliably catch:

// package.json
{
  "imports": {
    "#/*": "./*"
  }
}
// tsconfig.json
{
  "compilerOptions": {
    "module": "nodenext",
    "allowImportingTsExtensions": true,
    "rootDir": ".",
    "outDir": "."
  }
}
// index.ts
import { add } from "#/utils.ts";

This import resolves and is error-free during compile time and works as expected under node --experimental-strip-types. It also might easily be mistaken by the user as an import that will get rewritten to "#/utils.js", but it does not meet the criteria of being relative. If possible, we would like to catch these kinds of footguns, where something works under node --experimental-strip-types but breaks when compiled to JS. (I think it’s possible to catch this, but some experimentation is needed—it gets tricky with conditions and pattern trailers like "#/*.ts": "...".)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants