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

ModuleResolution Node16/NodeNext fails without extension starting on 5.5.0-dev.20240413 #58214

Closed
Noriller opened this issue Apr 16, 2024 · 8 comments
Labels
Not a Defect This behavior is one of several equally-correct options

Comments

@Noriller
Copy link

πŸ”Ž Search Terms

moduleResolution node16 nodeNext import extension

πŸ•— Version & Regression Information

  • This changed between versions 5.5.0-dev.20240413 and 5.5.0-dev.20240415 (last one checked)

⏯ Playground Link

No response

πŸ’» Code

type Something1 = typeof import('@some/package/path/to/file');
type Something2 = typeof import('@some/package/path/to/file.js');

πŸ™ Actual behavior

Something1 resolves to any
Something2 resolves to Something

πŸ™‚ Expected behavior

Something1 resolves to Something
Something2 resolves to Something

Additional information about the issue

This happens on the latest nightly versions and on moduleResolution Node16/NodeNext.

From what I could get, it started on 5.5.0-dev.20240413.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Apr 16, 2024

We need a complete repro here; the package.json of @some/package, its contents, the importing package, and the tsconfig for the importing file all matter

@RyanCavanaugh RyanCavanaugh added the Needs More Info The issue still hasn't been fully clarified label Apr 16, 2024
@Noriller
Copy link
Author

https://stackblitz.com/~/github.com/Noriller/ts-issue-58214

I hope this one helps.

In the test.d.ts with the current typescript@next and same jsconfig (its a legacy project, using mostly jsdocs, but with the problem in a d.ts file)

If changing ts version with:

npm i -D typescript@5.5.0-dev.20240412

then both work

@RyanCavanaugh RyanCavanaugh removed the Needs More Info The issue still hasn't been fully clarified label Apr 16, 2024
@jakebailey
Copy link
Member

This bisects to #57896. @andrewbranch

(I'm not an expert, but it seems strange that you'd need an extension when the d.ts file is effectively CJS, importing another CJS file, but importing via a JS file is already funky.)

@andrewbranch andrewbranch added the Needs Investigation This issue needs a team member to investigate its status. label Apr 17, 2024
@andrewbranch andrewbranch added this to the TypeScript 5.5.0 milestone Apr 17, 2024
@andrewbranch andrewbranch self-assigned this Apr 17, 2024
@andrewbranch
Copy link
Member

Guessing I accidentally made ImportTypes look like an ESM-mode source location, when they were previously kept CJS-mode for backward compatibility reasons. Will take a look. Thanks!

@andrewbranch andrewbranch removed their assignment Apr 18, 2024
@andrewbranch andrewbranch removed the Needs Investigation This issue needs a team member to investigate its status. label Apr 18, 2024
@andrewbranch andrewbranch removed this from the TypeScript 5.5.0 milestone Apr 18, 2024
@andrewbranch andrewbranch added Working as Intended The behavior described is the intended behavior; this is not a bug Not a Defect This behavior is one of several equally-correct options and removed Working as Intended The behavior described is the intended behavior; this is not a bug labels Apr 18, 2024
@andrewbranch
Copy link
Member

andrewbranch commented Apr 18, 2024

Your jsconfig.json is invalid:

Option 'module' must be set to 'Node16' when option 'moduleResolution' is set to 'Node16'

module is defaulting to es2015 because of target, which is causing everything to resolve under ESM rules (since everything will emit as ESM). I’d argue this is probably more logically consistent than what we used to do, where the module resolution implementation made a lot more assumptions about module format and emit.

I guess we could consider making --moduleResolution node16 imply --module node16 instead of issuing an error, but I think we did the error on purpose so that people would notice we were making a breaking change when we started requiring these options be used together in #54567.

@Noriller
Copy link
Author

Noriller commented Apr 19, 2024

That works for the import, but adding "module = node16" breaks for .js files "exporting" jsdoc types.

I've updated the example with that case (link again)

When a .js file don't import/export anything (in this one case it's still using module.exports) then the declared types can be used globally as if they were declared in a d.ts file.

Using "module = commonjs" apparently fixes the problem I had earlier while keeping other uses working, but it still gives an error in the jsconfig file.

I also had other projects that did have modules and that we do have types declared in a separate file.

BTW, about the error, since package.json also has a "module" option, maybe too much, but adding a "in this config file" to that type of error might make it clearer.

As for the case of exporting jsdoc types for global use, I'm not sure what should be the intended behavior.

@andrewbranch
Copy link
Member

You can set "moduleDetection": "legacy"

@typescript-bot
Copy link
Collaborator

This issue has been marked as "Not a Defect" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Not a Defect This behavior is one of several equally-correct options
Projects
None yet
Development

No branches or pull requests

5 participants