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

Silence “Cannot require an ESM file” error in type-only imports (and ImportTypeNodes) #53426

Closed
wants to merge 3 commits into from

Conversation

andrewbranch
Copy link
Member

Closes #52529

In Node, when a require call resolves to an ES module, loading/evaluation will crash because ES modules cannot be loaded synchronously. TypeScript issues a corresponding checker error whenever an import/require statement is written that would result in such a require of an ES module.

However, this checker error was also issued in cases where no require call is emitted at all, such as in type-only imports (import type { T } from "./mod") and import types (type T = import("./mod").T).

The error is only important insofar as it protects against a runtime error in Node, so I don’t think there’s any reason to issue it on constructs that are clearly and unambiguously non-emitting.

Why an ambient context alone does not silence the error

In my first pass at this, I also silenced the error on regular import statements that occur in declaration files, since declaration files themselves do not emit to JS. However, regular (non-type-only) import statements in declaration files ideally represent similar import statements in a JS implementation counterpart, so a user who includes a declaration file containing a suspicious import statement like that probably has reason to believe that the corresponding implementation has a problem, and should be warned about it.

Contrast to resolution-mode

The effect of this PR should not be confused with the effect of the (nightly-only) resolution-mode import assertion. Silencing this error allows CommonJS files to access types from ESM files, but only in cases where the CommonJS resolution algorithm resolves the module specifier to an ESM file, as would happen when writing an import of an ESM-only package—this PR does not change the result of any module resolutions. The resolution-mode import assertion, on the other hand, actually changes which resolution algorithm is used.

@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Mar 21, 2023
@weswigham
Copy link
Member

weswigham commented Mar 21, 2023

there’s any reason to issue it on constructs that are clearly and unambiguously non-emitting.

Instead, they make adding formats a breaking change where they otherwise aren't. To begin. Suppose we have

// @filename: node_modules/mypkg/package.json
{ "name": "mypkg", "type": "module", "exports": "./main.js" }
// @filename: node_modules/mypkg/main.js
export default "whatever";
// @filename: node_modules/mypkg/main.d.ts
const __default: string;
export default __default;
// @filename: usage.ts
// do note, without a package.json, this file is cjs
import("mypkg").then(p => console.log(p.member)); // JS is all good, cjs `import` resolves like esm
interface MyThing {
  x: typeof import("mypkg").default // currently an error (this `import` resolves like a `require`), in this proposal not an error
}

if this PR is in place, this is not an error. OK. Now suppose mypkg does the supposedly backwards-compatible thing and adds an explicit cjs entrypoint of the same shape, to help its' consumers out:

// @filename: node_modules/mypkg/package.json
{ "name": "mypkg", "type": "module", "exports": { "import": "./main.js", "require": "./main.cjs" } }
// @filename: node_modules/mypkg/main.js
export default "whatever";
// @filename: node_modules/mypkg/main.d.ts
declare const __default: string;
export default __default;
// @filename: node_modules/mypkg/main.cjs
module.exports.default = "whatever"; // output from transforming the esm downlevel
// @filename: node_modules/mypkg/main.d.cts
declare const __default: string;
export default __default;
// @filename: usage.ts
// do note, without a package.json, this file is cjs
import("mypkg").then(p => console.log(p.member)); // JS is all good, cjs `import` resolves like esm
interface MyThing {
  x: typeof import("mypkg").default // meaning suddenly changes! now resolves to the cjs entrypoint, rather than the esm one!
}

If we did this, we make what should be a non-breaking, safe change into a breaking change for TS consumers. That won't be good for the ecosystem. :(

@Andarist
Copy link
Contributor

Are there any caveats related to this in relation to the dual package hazard? Can a type from an inaccessible ESM module leak into the module graph and end up conflicting with its CJS counterpart? It seems somewhat unlikely since, after all, CJS isn't available in this situation... Although, there is probably a convoluted test case to be created here that would involve an ESM-only root entry and dual subentry?

@andrewbranch
Copy link
Member Author

You can always find a way to make adding a format a “breaking change.” Take the reverse scenario: you have an ESM file and a CJS dependency:

// @Filename: main.mts
import ts from "typescript";

Then TypeScript adds an ESM entrypoint. Suddenly the resolution of the import changes! This scenario is way more realistic since it involves a plain old import declaration of a value instead of an import type, and an addition of ESM to existing CJS instead of the reverse (most people who have decided to ship an ESM-only package are not being convinced to add CJS at this point).

It doesn’t make a lot of sense to error on one of these scenarios and not the other, but I don’t see anyone suggesting that we should raise an error every time a (type-only, even) ESM import resolves to a CJS file. Resilience against the structure of your dependencies changing when you update them is not really an invariant we have; just this one corner happened to be guarded against it by a too-conservative error message. And I maintain that the sequence of events that would have to happen to cause a real problem, where it would have been an error before, will be exceedingly rare.

@Andarist the type-level dual package hazard can be affected by using resolution-mode generally, or even just having CJS and ESM files and re-exporting stuff you import in them. Same as the value-land concern, really. I don’t think this PR moves the needle one way or the other on it.

@Andarist
Copy link
Contributor

@Andarist the type-level dual package hazard can be affected by using resolution-mode generally, or even just having CJS and ESM files and re-exporting stuff you import in them. Same as the value-land concern, really. I don’t think this PR moves the needle one way or the other on it.

Hm, I feel like it might move that needle a little bit - as the user might not be aware that at a given moment TS "silently" allows them to access ESM-only files at type-level. I agree that the dual package hazard can always happen in convoluted scenarios but at least in those situations the runtime-level hazard and type-level hazard stay kinda the same as people are only allowed to import/require things from modules of a given type. This introduces a new subtle way to end up in this situation.

I don't necessarily mean that this is a bad thing - perhaps the DX improvement of this change is worth it. I remember my own initial reaction when wanting to import a type from ESM into CJS... I wanted this to be possible :P I think though that the needle is moved here - how big of a move it is? Dunno 🤷

@weswigham
Copy link
Member

It doesn’t make a lot of sense to error on one of these scenarios and not the other, but I don’t see anyone suggesting that we should raise an error every time a (type-only, even) ESM import resolves to a CJS file

That's because node acknowledges swapping a cjs dependency to esm almost always is a breaking change and should almost always be a major version bump (because, minimally, it changes the shape of your default exports)? Nobody needs an error on it because the community expects it to break and (mostly) versions appropriately. Migrating from cjs to esm (or just adding esm) can often be a breaking change, and should be done in a major revision, while adding cjs to an esm only package should basically never be a break (since you never had any functioning cjs consumers), so can be shipped in a patch release.

@andrewbranch
Copy link
Member Author

andrewbranch commented Mar 22, 2023

I see your point, I just don’t think it’s worth the poor DX we have today. When we issue an error on some code, it’s generally expected that the code in question is wrong or dangerous at runtime (or creates a type-level inconsistency that could allow wrong or dangerous runtime code to type check down the line), such that fixing it would be worth the user’s time. Erroring on code that might change meaning if they take an explicit action in the future (at which point we could error again if it creates a problem!) is clearly not part of what users think they’re signing up for when they run our type checker. It sounds like a reasonable lint rule to me. For us to justify erroring (with no workaround, for non-nightly users!) over a hypothetical future problem, we’d have to think that problem is quite big and/or likely, and I don’t see any evidence that it is. How many npm packages can you name that shipped ESM first and added CJS later?

@weswigham
Copy link
Member

Here's a big question, though: Who's referencing the types of an esm-only package in cjs, but not its' implementation? If you think "well, this is useful alongside dynamic import in cjs, maybe", as the dynamic import will follow esm resolution rules and esm conditions, while the type-only imports will still be following cjs ones... won't that just be confusing? Basically all the reports I've seen have been from people trying to solve our compiler error saying that the type isn't reachable to a non-esm-format import, and trying to make that work without an actual esm-format-searching-import is riddled with pits of failure. Like, yeah, so long as there's no cjs implementation alongside, with this we'd suddenly find something, but when there is? Error's still there. This adds a lot of potential ecosystem burden on upgrades and doesn't seem like a complete or reliable solution to the general problem of reaching esm format types from cjs (which, again, 90% of the time is only a problem because we say it is - if we, say, choose to generate resolution-mode imports in declaration emit, despite how they're not stable yet, this problem would be invisible to most people, since they'd never get the un-nameable type error in the first place)...

@andrewbranch
Copy link
Member Author

@ShogunPanda @ef4 @Andarist have all expressed that they’ve tried to do this; can any of you elaborate on your use case a bit?

@Andarist
Copy link
Contributor

I tried to create a declaration file for https://github.com/gxmari007/vite-plugin-eslint . It's a plugin for Vite and Vite is esm-only. However, Vite is able to load CJS plugins and that plugin annotates its return value with import('vite').Plugin to get contextual typing and validation. This one only relies on structural typing (and possibly on duck-typing in Vite itself) and it seems to be OK.

Later I've thought about cases like:

// .d.cts
import type { Nominal } from 'esm-only'
export declare function accept(input: Nominal): void

and at that point, I lost my confidence in being able to do this a good thing. OTOH, you could have an API like that 🤷 it's possible to use a dynamic import in CJS to load ESM files. How that should be typed?

@ef4
Copy link

ef4 commented Mar 23, 2023

Who's referencing the types of an esm-only package in cjs, but not its' implementation?

I'm using its implementation sometimes. Simplified example:

import type { MinifyOptions } from 'terser';

export async function build(code: string, minify?: MinifyOptions) {
  if (minify) {
    let terser = await import('terser'); 
    code = terser.default.minify(code, minify);
  }
  return code;
}

The type is part of my type, the implementation should only load when needed.

Consider also that this pattern can be used to guard code that doesn't even eval in the current environment, so using the dynamic import() over static import is not just an optimization, it can be necessary for correctness.

@weswigham
Copy link
Member

weswigham commented Mar 23, 2023

I'm using its implementation sometimes.

Great! And via dynamic import, too! Wouldn't it be odd that the only way to access the types of said dynamic import were via a lookup that doesn't follow the same lookup rules as said dynamic import actually has, and, in fact, may in all likelihood still be unable to access those types?

@ef4
Copy link

ef4 commented Mar 23, 2023

I also tried accessing those types directly through the same mechanism. That is, surely typeof import('terser') should follow the same rules as import('terser'):

type Terser = typeof import('terser');

But this fails in the same way as the static import. Even though it's clear that TS can locate the types, because this workaround actually works:

async function loadTerser() {
  return await import('terser');
}
type Terser = Awaited<ReturnType<typeof loadTerser>>;

@andrewbranch
Copy link
Member Author

andrewbranch commented Mar 23, 2023

That is, surely typeof import('terser') should follow the same rules as import('terser')

Tragically, it does not, for backward-compatibility reasons. The former syntax existed in TypeScript and had CJS-y resolution semantics long before Node decided dynamic import in CJS files would have ESM-y resolution semantics.

@ShogunPanda
Copy link

In my case I was just importing a type (an interface, to be precise) in the file and then write some code. Nothing else.

The problem was that I must use CJS while the referenced module is ESM.

@ef4
Copy link

ef4 commented Mar 23, 2023

Tragically, it does not, for backward-compatibility reasons.

If the syntax is claimed already then oh well. But what is the recommended workaround then? I can keep using a function that I never call just to infer the type off of:

async function loadTerser() {
  return await import('terser');
}
type Terser = Awaited<ReturnType<typeof loadTerser>>;

but maybe there is something else?

@ef4
Copy link

ef4 commented Mar 23, 2023

Actually I guess I should ask: does the type inference example above actually follow the same resolution rules as the implementation would? I hope yes. If not, that seems WAT.

@andrewbranch
Copy link
Member Author

But what is the recommended workaround then?

There is no recommended workaround, hence this PR. The only workaround that exists is currently only available in nightly versions of TypeScript, because it reuses some syntax in type-space that TC39 is still sorting out the meaning of in value-space, and there was significant enough pushback when we adopted it that we made it unavailable outside of typescript@next. So yeah, unless you’re using nightly, there is no other workaround, at least until the future of import assertions / import reflection is known.

Actually I guess I should ask: does the type inference example above actually follow the same resolution rules as the implementation would?

Yes.

@weswigham
Copy link
Member

weswigham commented Mar 27, 2023

reuses some syntax in type-space that TC39 is still sorting out the meaning of in value-space

The meaning is less important than the syntax 😂

There's consensus to loosen the spec restrictions that we got a complaint the we were "violating" with resolution-mode already - it'd be allowed at runtime according to the new version, so that complaint definitely doesn't hold water anymore. The issue is the syntax changes paired with the relaxation. T.T

TBH, I think we can un-nightly resolution-mode specifically at this point, but we need to go back to erroring on all assertions (nee attributes) until the proposal advances again (and parse with besides)...

@sandersn sandersn added this to Not started in PR Backlog Mar 29, 2023
@sandersn sandersn moved this from Not started to Waiting on author in PR Backlog Mar 29, 2023
@andrewbranch
Copy link
Member Author

The proposal reached conditional Stage 3 the day after I made my comment, so we may be able to just move forward with with soon. Once that’s the case, I’m fine to redo this PR as changing the error messages instead of silencing them. Right now they’re incredibly confusing for import types:

type Terser = typeof import('terser');
// The current file is a CommonJS module whose imports will produce 
// 'require' calls; however, the referenced file is an ECMAScript module 
// and cannot be imported with 'require'. Consider writing a dynamic 
// 'import("terser")' call instead.

@andrewbranch
Copy link
Member Author

I’m guessing/hoping we’ll be able to land import attributes fairly soon, so I’m going to close this

PR Backlog automation moved this from Waiting on author to Done Jun 20, 2023
@andrewbranch andrewbranch deleted the feature/52529 branch June 20, 2023 17:40
@slorber
Copy link

slorber commented Jun 29, 2023

@andrewbranch just wondering why closing this issue if it's not already implemented 😅 I'd like to be notified once the solution is out personally.

@RyanCavanaugh wasn't as confident it will land soon (#53656 (comment)), anything changed recently regarding this proposal?

Given the Stage 3 -> 2 regression on the original proposal, we might hold off on this until it's more like Stage 3.5


When you mean import attributes in the context of this issue's problem, you mean something like this?

import type { Plugin } from 'unified' with { "resolution-mode": "import" };

Related links others might find useful:

@andrewbranch
Copy link
Member Author

The proposal has been progressing positively: tc39/proposal-import-attributes#137. You should subscribe to the other issues you linked to be notified. This PR was an experiment for sidestepping the need for import attributes altogether and it failed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

Should type-only import resolving to ES module from CJS module be allowed?
7 participants