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

resolution-mode Feedback #49055

Closed
DanielRosenwasser opened this issue May 10, 2022 · 10 comments
Closed

resolution-mode Feedback #49055

DanielRosenwasser opened this issue May 10, 2022 · 10 comments
Labels
Discussion Issues which may not have code impact

Comments

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented May 10, 2022

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

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

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

jaydenseric commented Jun 10, 2022

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

Xunnamius commented Oct 15, 2022

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

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

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

johan commented Jun 17, 2023

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

valerii15298 commented Jul 1, 2023

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

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

schnz commented Jul 10, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Issues which may not have code impact
Projects
None yet
Development

No branches or pull requests

10 participants