Skip to content

Conversation

saschanaz
Copy link
Contributor

I don't understand the reason behind #2108 (comment), this works perfectly?

Copy link
Contributor

Thanks for the PR!

This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged.

@saschanaz saschanaz marked this pull request as draft August 25, 2025 21:57
@saschanaz saschanaz marked this pull request as ready for review August 25, 2025 22:00
Copy link
Contributor

@Bashamega Bashamega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks to me like it just added 2 new letters and the type. I don't see why this is beneficial.

@HolgerJeromin
Copy link
Contributor

HolgerJeromin commented Aug 26, 2025

Reducing to a type import is nice.

From @jakebailey

Note that the PR moved the file to .ts; you can't import a .d.ts file straight up like that without also enabling flags that allow importing TS files.

Has this flag been set?

But changing to an .d.ts import feels fishy because TS compiler will search for a corresponding .d.ts file when importing stuff (types or not) from a .js file (with or without the extension).

@saschanaz
Copy link
Contributor Author

saschanaz commented Aug 26, 2025

We are not adding a TypeScript file import in runtime (which would require node.js experimental flag), or I'm still confused what he tried to say...

@HolgerJeromin
Copy link
Contributor

HolgerJeromin commented Aug 26, 2025

We are not adding a TypeScript file import in runtime (which would require node.js experimental flag), or I'm still confused what he tried to say...

Jake's comment:

Note that the PR moved the file to .ts; you can't import a .d.ts file straight up like that without also enabling flags that allow importing TS files.

I think he meant the TS flag/config allowImportingTsExtensions which allows importing .ts files. Which is otherwise not valid even for type-only imports.

@jakebailey
Copy link
Member

This is pretty atypical. Auto imports will never suggest these paths, and for more real published packages, you'd want to avoid this because you'd end up with files that are unimportable because there's no JS file left to do it. I don't even remember if these d.ts input files are even written back out to the output...

We are not adding a TypeScript file import in runtime (which would require node.js experimental flag), or I'm still confused what he tried to say...

FWIW running TS directly is no longer experimental in 22/24, so I would honestly consider just using a higher minimum in the repo and just use that feature (with requisite tsconfig options). It works well.

@saschanaz
Copy link
Contributor Author

saschanaz commented Sep 20, 2025

This is pretty atypical.

I find it also atypical to have a type-only library but not use d.ts.

Auto imports will never suggest these paths

Sounds like a bug.

and for more real published packages, you'd want to avoid this because you'd end up with files that are unimportable because there's no JS file left to do it.

Real published packages would have a separate compiled d.ts library.

I don't even remember if these d.ts input files are even written back out to the output...
FWIW running TS directly is no longer experimental in 22/24, so I would honestly consider just using a higher minimum in the repo and just use that feature (with requisite tsconfig options). It works well.

And if we want to target Node.js native typescript syntax support, then such type-only import should be removed/ignored by Node.js or it would be a bug.

@jakebailey
Copy link
Member

jakebailey commented Sep 22, 2025

Maybe it's more clear to say ".d.ts files describe actual importable modules on disk". It is not advised by the TS team to write .d.ts files for any other purpose. Irritatingly I can't find the doc that says "don't put helper types in declaration files".

Auto import suggests the paths because it thinks that the declaration files accurately describe that the import will work. The type import "working" is really just a technicality.

@saschanaz
Copy link
Contributor Author

Maybe it's more clear to say ".d.ts files describe actual importable modules on disk". It is not advised by the TS team to write .d.ts files for any other purpose. Irritatingly I can't find the doc that says "don't put helper types in declaration files".

Auto import suggests the paths because it thinks that the declaration files accurately describe that the import will work. The type import "working" is really just a technicality.

Maybe the work item is to have a documented guideline with reasons? It's fine if the official stance is that it's not a supported feature (and it can break anytime), but it should really be documented in that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants