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

bug: TypeScript 4.8 unable to discover @preact/signals for auto import #111

Closed
motss opened this issue Sep 9, 2022 · 8 comments
Closed

Comments

@motss
Copy link

motss commented Sep 9, 2022

Description

Not sure if this is the right place to post this TypeScript specific issue but I found out that auto importing @preact/signals fails in VSCode when using latest TypeScript 4.8. Auto importing works just fine when downgrading to TypeScript 4.7.

Affected TypeScript version: 4.8.3
Affected @preact/signals version: 1.0.3

Steps to reproduce

  1. Create a .ts or .tsx file with simple TSConfig file.
  2. Write signals to trigger VSCode's autocompletion to auto import signals from @preact/signals

Expected behavior

Auto imports should work when typing signals

Actual behavior

Auto imports fails to auto import anything from @preact/signals when using TypeScript 4.8.

@marvinhagemeister
Copy link
Member

Thanks for reporting this issue.

To be honest this sounds more like an issue in TypeScripts language server and not something in the signals library. I tried to reproduce it on my end, but auto importing works fine for me in TS 4.8. I'd recommend reaching out to their issue tracker. A quick search reveals a few issues with project references and auto imports.

@motss
Copy link
Author

motss commented Sep 9, 2022

@marvinhagemeister Thanks and I've moved this to the TypeScript Github repo.

@andrewbranch
Copy link

I suspect this is at least in part a TypeScript bug, but the first thing I noticed in my at-a-glance investigation is that the types key of the exports of your package.json is last in the object. That’s going to make it impossible for TypeScript to see. If the importing script is CJS, it will find it via following the require lookup and swapping the .js extension to .d.ts, but if the importing script is an ES module, it will use the import lookup and find nothing. You probably want to have a dedicated signals.d.mts for ESM, and then remove the types key altogether.

@frehner
Copy link

frehner commented Sep 10, 2022

While we’re talking about package exports, though, maybe also consider adding “module” as well to help ensure there’s only ever one instance of signals in the end bundle. :)

@marvinhagemeister
Copy link
Member

@andrewbranch Thanks for taking a look, that's much appreciated! Tried it out various approaches and I'm still unable to reproduce the described issue on my end.

  • vscode vs vscode insiders
  • With "type": "module" in packge.json and without
  • With tsconfig.json and without it
    • With "module": "NodeNext" and "moduleResolution": "NodeNext"
    • With "module": "ESNext"
  • With .mjs, .mts, .ts, .tsx or .js
  • Put "types" field first

Regardless of the combinations I've tried, IntelliSense was able to find the correct definition file. It didn't break. Only when I removed the "types" fields vscode wasn't able to find the correct types anymore.

When adding the imports and building with tsc's --explainFiles flag, the definition files are always found too, regardless of if the project is in ESM or not.

@frehner I'm curious to learn more. Why would adding the module entry help ensure only one version is imported? Do you have more details on that?

@frehner
Copy link

frehner commented Sep 10, 2022

Yeah! I cite 3 sources in the exports section here

https://github.com/frehner/modern-guide-to-packaging-js-library#define-your-exports

but the TLDR is that bundlers came to the agreement that they would use the module field in package exports when bundling, regardless of whether a package was imported or required, to help dedupe the package and also help prevent the dual package hazard.

@andrewbranch
Copy link

Tried it out various approaches and I'm still unable to reproduce the described issue on my end.

It turns out it’s working due to a bug in TypeScript, which I think we are unlikely to fix, but we agreed that the exports of this package are not quite right with respect to TypeScript.

Because you have no default export, I think a single .d.ts file like you have now is ok. Just move types to the top to make sure we don’t break you by fixing a bug 😄

@marvinhagemeister
Copy link
Member

@andrewbranch That sounds perfect 👍 We'll move the types entry to the top of the list. Thanks again for reaching out that's really awesome!

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

No branches or pull requests

4 participants