microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.06k stars 12.37k forks source link

`resolution-mode` Feedback #49055

Closed DanielRosenwasser closed 1 month ago

DanielRosenwasser commented 2 years ago

For TypeScript 4.7, we pulled back the resolution-mode import assertion from import type syntax. This was based in part on feedback around the feature being a bit outside the spirit of import assertion syntax (https://github.com/microsoft/TypeScript/issues/48644); however, we also just don't want to add features if we're not 100% convinced that they're going to be used. This was discussed a bit at https://github.com/microsoft/TypeScript/issues/48686.

So if you try to use resolution-mode as follows

// Resolve `pkg` as if we were importing with a CommonJS `require()`
import type { TypeFromRequire } from "pkg" assert {
    "resolution-mode": "require"
};

// Resolve `pkg` as if we were importing with an ES module `import`
import type { TypeFromImport } from "pkg" assert {
    "resolution-mode": "import"
};

export interface MergedType extends TypeFromRequire, TypeFromImport {}

you'll get the following error:

Resolution mode assertions are unstable. Use nightly TypeScript to silence this error. Try updating with 'npm install -D typescript@next'.

Maybe you're using this because you're trying to bridge a CJS/ESM codebase, or maybe you're trying to provide some sort of meta-library that does that. We're not entirely sure!

So if you run into this error, what are you trying to do? What do you need it for? Is there a syntax you'd prefer to use, or is it fine as-is?

Jamesernator commented 2 years ago

Is resolution-mode supported for declare module and such? Because while I don't know that I'd need import type ... assert { "resolution-mode": "..." } much in regular code, if I were adding a local type definition for a third party module then doing:

declare module "someUntypedPackage" assert { "resolution-mode": "import" } {
    import type Blah from "someDependentPackage" assert { "resolution-mode": "import" };

    export function foo(blah: Blah): string;
}

would certainly be helpful.

This was based in part on feedback around the feature being a bit outside the spirit of import assertion syntax

I do agree with this, like it's not really an assertion, it actively changes the mode. Something like import type ... from "..." using { "resolution-mode": "..." } would make more sense.

weswigham commented 2 years ago

Is resolution-mode supported for declare module and such?

Not at present, but we're aware of the potential need. Right now am ambient module is assumed to define the specifier for both resolution modes, which is odd, but consistent, at least. If you break it out into actual module files, you can get the per-mode conditional definitions node allows.

jaydenseric commented 2 years ago

So if you run into this error, what are you trying to do? What do you need it for?

graphql-upload is currently CJS. It has a dependency fs-capacitor that used to be CJS, but now is pure ESM. While graphql-upload will be pure ESM in the nearish future, I would like to be able to update the fs-capacitor dependency before then by dynamically importing the pure ESM within the only graphql-upload function that uses it (graphql-upload/processRequest.js) which fortunately is already an async function.

The problem is, that there is a JSDoc type import("fs-capacitor").WriteStream that would be broken because the module the type is in is CJS, and fs-capacitor is ESM:

https://github.com/jaydenseric/graphql-upload/blob/b1cdd2a913c5394b5ff5f89b28d79b949b0bdde5/processRequest.js#L360-L362

Using TypeScript nightly this works:

import("fs-capacitor", { assert: { "resolution-mode": "import" } }

There needs to be a solution for this, and it appears outside of using TypeScript nightly (it's not viable for a published package to expect all users to also update to TypeScript nightly) there is none.

Is there a reason that TypeScript couldn't treat import() types as if they are all dynamic imports, as the syntax suggests? A dynamic import within CJS or ESM can consume either CJS or ESM just fine. It seems to currently default to the mode that the import is actually a require(), when it makes more sense to default to the assumption it's an import()?

Xunnamius commented 1 year ago

I build little utility libs written in TypeScript but distributed as CJS packages, and while I truly wish it were practical to migrate everything to ESM and force downstream consumers to migrate as well, there are a few practical roadblocks to this like Node's still-experimental vm modules and the resulting lack of full ESM support / segfaults in Jest and other testing frameworks. Most of the time I can get away with pinning the CJS versions of packages that have since migrated to ESM. Other times I use dynamic imports to bring in ESM packages. Either way, with a well-configured exports property in package.json, I keep CJS and ESM consumers happy without having to provide a dual CJS-ESM package.

Doing things this way isn't actually a problem with TypeScript for me, since I use moduleResolution: Node, typesVersions, and babel to transpile it all down to CJS anyway.

But now I want to try using moduleResolution: NodeNext, mostly for conditional entry resolution and conditional exports support; specifically, I want support for multiple entry points each with their own types resolved via the type condition without having to rely on typesVersions. Everything has been working for me so far, but today I've finally come across a case where I need to import a type from a dynamically imported ESM package and then export interfaces that use that type to downstream consumers.

I was pleasantly surprised when import { visit } from 'unist-util-visit' gave me the following error:

The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module ... Consider writing a dynamic 'import("unist-util-visit")' call instead. ...

That's a nice warning, usually I have to remember which imports are ESM and which are CJS myself.

However, I was confused when I changed it to let visit: typeof import('unist-util-visit').visit and TypeScript was still giving me the same error. This is a dynamic type-only import, why am I getting complaints about "... produce 'require' calls ..." that will never be produced? At the top of another file, I have the line import type { BuildVisitor } from 'unist-util-visit/complex-types', and this too gives me the same error about require calls.

A bit of Googling and reading through GitHub issues lead me to the resolution-mode assertion, and then to this issue. Though I've read through the discussions that preceded this issue, I'm still not quite clear on why TypeScript doesn't understand that my type-only imports don't produce require calls.

Anyways, I considered littering my code with @ts-expect-error above all the type-only imports in lieu of the resolution-mode assertion, but instead I think I'll just go back to the old moduleResolution: Node setting and rely on typesVersions for pseudo-exports support until things stabilize further. Since any downstream consumers using TypeScript are probably not using moduleResolution: NodeNext, I was going to have to keep typesVersions around regardless.

Growing pains of having two partially-compatible module systems, I guess šŸ˜„.

mscharley commented 1 year ago

I haven't found a good way to import types from an ESM-only module into a CJS module. This seems to be the best current option, but relies on @ts-expect-error which feels pretty brittle:

// @ts-expect-error
import type { LikeConnectionSource, LikeEdgeSource } from './resolvers/LikeConnection.mjs' assert { "resolution-mode": "import" };

Or, require typescript nightly.

weswigham commented 1 year ago

@DanielRosenwasser the concerns that prompted #48644 no longer apply under the new version of the proposal (not that I think that ever really needed to apply to a typespace construct anyway). Do we have a rough forecast from our TC39 meeting on when the proposal may re-advance to stage 3? I imagine when it does (and we support the new keyword), we'll be able to un-nightly this.

johan commented 1 year ago

If this syntax (or any syntax ā€“ there is a lot of demand), can be used to get exact const type shapes of resolveJsonModule json imports (#32063) instead of a loosey-goosey { "key1": string, "key2": string } type, that would be super useful.

Maybe that's hard, or out of scope for what you're trying to do here, but it would have lots of happy users currently squeaking by without it by way of using external input transform tools like unplugin-json-dts instead, that literally just wraps the whole referenced json files in the typescript code required to export an as const type shape, for lack of an in-typescript solution.

valerii15298 commented 1 year ago

I need to use assert to import types from ESM project into my CJS project and bcs if it I also need to use nightly typescript... It is extremely inconvenient, bcs I just wanna import types and use stable typescript version but I cannot bcs of this :( And seems there is no any good workaround...

import type KeycloakAdminClientRaw from "@keycloak/keycloak-admin-client" assert { "resolution-mode": "require" };
export type KeycloakAdminClient = KeycloakAdminClientRaw;

import type GroupRepresentationRaw from "@keycloak/keycloak-admin-client/lib/defs/groupRepresentation" assert { "resolution-mode": "require" };
export type GroupRepresentation = GroupRepresentationRaw;

import type EventRepresentationRaw from "@keycloak/keycloak-admin-client/lib/defs/eventRepresentation" assert { "resolution-mode": "require" };
export type EventRepresentation = EventRepresentationRaw;

import type AdminEventRepresentationRaw from "@keycloak/keycloak-admin-client/lib/defs/adminEventRepresentation" assert { "resolution-mode": "require" };
export type AdminEventRepresentation = AdminEventRepresentationRaw;

import type UserRepresentationRaw from "@keycloak/keycloak-admin-client/lib/defs/userRepresentation" assert { "resolution-mode": "require" };
export type UserRepresentation = UserRepresentationRaw;

As you see I need to reexport all types from library which I use, simply bcs typescript doesn't allow me import types directly at all(assuming if I wanna use stable ts version without assert)...

morganney commented 1 year ago

Dual packages are a real thing. You run into this problem anytime you want to import type from an .mts file into a .cts file, or vice versa.

This just needs to work with resolution-mode assertions, or whatever other syntax you want to throw at it, and without needing to

Use nightly TypeScript to silence this error. Try updating with 'npm install -D typescript@next'.

schnz commented 1 month ago

Is there a reason that this is still open? As far as I can tell, this is no longer a nightly-only feature.

But if you are still looking for feedback: This issue taught me a new TypeScript feature that I wasn't aware of and it helped to resolve an issue where I converted a project from CJS to ESM and I needed the ability to import a type as CJS in order to stay compatible with a CJS-only library which transitively imports this type as CJS and I need to stay compatible with it.

The only other solution would have been to convert the library to emit both CJS and ESM code which would have been possible but would have implied a larger time-effort and I would rather spend that time in a few weeks from now than now.