signals-react: Wrong import path in dist/signals.d.ts causing TypeScript errors

[fabianmichaelATform4]

Hi,

we've discoverd an issue in dist/signals.d.ts that is causing TypeScript errors when building our app. It contains the following line:

import { useSignal, useComputed, useSignalEffect } from "../runtime/src/index";

That causes the source file ../runtime/src/index to be included by the TypeScript compiler when building our app causing the TypeScript errors. If I manually change the import path from src to dist it works fine

[XantreDev]

Probably it should be just ../runtime

[rschristian]

is causing TypeScript errors

And what errors are those?

[paul-sachs]

I've also seen similar problems upgrading to most recent version. There errors I've seen are:

error TS7016: Could not find a declaration file for module 'use-sync-external-store/shim/index.js'. 'node_modules/.pnpm/use-sync-external-store@1.2.0_react@18.2.0/node_modules/use-sync-external-store/shim/index.js' implicitly has an 'any' type.

This causes problems since @types/use-sync-external-store is a devDependency, it's not included by default.

[rschristian]

Enable skipLibCheck.

We shouldn't be adding @types/use-sync-external-store as a dep as it's not exposed to the user in any way, AFAIK.

[paul-sachs]

I have skipLibCheck set to true and it doesn't change anything, it still fails in the same place.

[rschristian]

That doesn't sound right. Where have you set it?

Edit: Unless TS doesn't consider that a lib definition. Ugh

[paul-sachs]

It doesn't:

Skip type checking all .d.ts files.

And since src/index isn't a .d.ts file, it goes boom.

[rschristian]

That sounds like a TS bug. Errors, even in .ts files, should not be returned to the user if skipLibCheck is set and the module is imported by a .d.ts file. Entire graph should be silenced from that point on.

[paul-sachs]

For reference, https://github.com/microsoft/TypeScript/issues/41883 discusses this in detail. I agree, skipLibCheck was the first thing i verified when looking at this bug.

[paul-sachs]

As it turns out @XantreGodlike was correct, if we just don't import the full filepath, the local package.json resolves properly.

[XantreDev]

@rschristian Dependency will be bundled if useing ../runtime or it will try to import it from runtime? If it will be bundled there should be no difference in behaviour and we can try to change imports

[rschristian]

Dependency will be bundled if useing ../runtime or it will try to import it from runtime?

Microbundle doesn't bundle types, no.

[XantreDev]

I am not about types. I am about this import, which moves to d.ts after type emission. https://github.com/preactjs/signals/blob/70ef4bb8694657ee43bcf2cf3f10732941c77f0b/packages/react/src/index.ts#L16

[rschristian]

https://www.npmjs.com/package/@preact/signals-react?activeTab=code

[XantreDev]

I know that now the source code of subpackage bundled. Will it be bundled if you replace the import from ../runtime/src/index to just ../runtime. I think there will not be any difference in behaviour in that case - but types will be correct

[rschristian]

I'm not quite sure what you're referring to, but in any case, best way to get an answer is to clone & try out whatever you're after. I may have done a fair bit with Microbundle but my memory of its behavior isn't perfect.

[XantreDev]

Nothing changes in terms of bundling if using relative import, but since runtime is not bundled initially ../runtime causes typescript error. @paul-sachs check if @preact/signals-react/runtime works ok in .d.ts. If it will - here is PR

[paul-sachs]

@XantreGodlike I can confirm that using @preact/signals-react/runtime works for my build. I wonder if it would make sense to make sure we don't ship .ts files to npm would guarantee these errors would be caught earlier. But the PR works, so that's good enough for me.

[rschristian]

I wonder if it would make sense to make sure we don't ship .ts files to npm

TS makes this a bit sketchy, but we (in the Preact org) generally prefer to ship the source files alongside built.