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

Support importing types from a .mts file from within a CommonJS file. #58195

Open
6 tasks done
Jason3S opened this issue Apr 15, 2024 · 9 comments
Open
6 tasks done

Support importing types from a .mts file from within a CommonJS file. #58195

Jason3S opened this issue Apr 15, 2024 · 9 comments
Labels
Help Wanted You can do this Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases
Milestone

Comments

@Jason3S
Copy link

Jason3S commented Apr 15, 2024

🔍 Search Terms

"commonjs", "import", "type", "module"

✅ Viability Checklist

⭐ Suggestion

Proposal: Allow importing pure types from .mts files from a CommonJS file.

The current behavior generates an unnecessary / incorrect error since no require will be emitted.

Current error:
image

Please note, future versions of Node will support requiring an ESM module if a there is not a top level await. See:

📃 Motivating Example

src/code.cts

import type { EntityId, Entity, User } from './models.mjs';

export function userName(user: User): string {
    return user.name;
}

src/models.mts

export type EntityId = string;

export interface Entity {
    id: EntityId;
}

export interface User extends Entity {
    name: string;
}

💻 Use Cases

  1. What do you want to use this for?
    In mixed code bases (especially as they transition from CommonJS to ESM) it is often necessary export the types from a module that are used in CommonJS files.
  2. What shortcomings exist with current approaches?
    All shared types must be declared in CommonJS files or inferred using Awaited.
  3. What workarounds are you using in the meantime?
    Declaring shared types in CommonJS files.
@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Apr 15, 2024

Duplicate #52529 edit: maybe?

@RyanCavanaugh
Copy link
Member

@andrewbranch points out that the concerns in the linked issue are specific to resolution hitting different targets in upstream packages due to revisions in their package jsons, but this argument doesn't apply at all to local relative paths

@andrewbranch
Copy link
Member

The currently intended behavior is that you’re supposed to silence this error by adding with { "resolution-mode": "import" } to assert that you’re doing what you’re doing on purpose. At minimum, we need to update this error message to suggest that workaround, but I was finding it hard to construct a message that adequately explains what’s wrong with this code, since I’m still somewhat skeptical that there’s anything wrong with this code.

@jakebailey
Copy link
Member

jakebailey commented Apr 15, 2024

I'm confused; isn't the context of this issue nodejs/node#51977, such that when that's "final", we need to have a new module resolution mode which re-allows require(ESM)? And that this issue isn't about anything else?

(Hm, maybe not, as the above was just a "side point" of sorts...)

@Jason3S
Copy link
Author

Jason3S commented Apr 15, 2024

@andrewbranch,

with { "resolution-mode": "import" }

This solves the my main issue. I did search for this, but wasn't able to figure it out based upon #53656. I was also trying to use verbatimModuleSyntax, but that doesn't work with CommonJS files.

I completely missed typescriptlang.org Stable Support resolution-mode in Import Types. I knew about with { type: "json" }.

By the way, the "value" of "resolution-mode" doesn't seem to matter for local files.

This works fine.
src/code.cts

import type { User } from './models.mts' with { "resolution-mode": "require" };

Which leads to the question of why resolution-mode is necessary for local files.

@jakebailey,

I'm confused; isn't the context of this issue nodejs/node#51977, such that when that's "final", we need to have a new module resolution mode which re-allows require(ESM)? And that this issue isn't about anything else?

(Hm, maybe not, as the above was just a "side point" of sorts...)

It is a "side point", I was using it as part of the argument that importing ESM files from a CJS file is necessary to support. Given that, it would also preclude the need for with { "resolution-mode": "import" }

@andrewbranch
Copy link
Member

Yeah, if that lands without a flag, there will be a new module mode / nodenext will be updated to support it without error. There’s nothing to do on that front yet.

@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases labels Apr 17, 2024
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Apr 17, 2024
@RyanCavanaugh
Copy link
Member

We should at least update the error message to suggest the resolution mode assertion

@Jason3S
Copy link
Author

Jason3S commented May 14, 2024

It looks like Node 22 includes require('./file.mjs') support: Support require()ing synchronous ESM graphs.

@andrewbranch
Copy link
Member

Under an experimental flag. nodenext won’t be updated until it lands unflagged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Help Wanted You can do this Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases
Projects
None yet
Development

No branches or pull requests

4 participants