WebReflection/usignal

Incomplete types

Closed this issue · 15 comments

  1. missing .peek on signal() result
  2. missing usignal/* types
  3. effect signatures do not reflect sync/async/core variants

for @webreflection/signal I ended up ditching automatic TS types creation and wrote it manually ... would you say that is better than types provided in here? Maybe I should just share the same file and add changes manually for the async export ... thoughts?

Manual vs generated types usually depends on:

  • how complex logic is (sometimes it's much easier to just ignore TS with it's multiple incompatibilities with JS)
  • whether you want internal implementation to be type checked vs just expose type for DX

For me as a lib consumer I care only about DX, so as long as types are in-sync with the docs and the usage I should not care.
But if I'd want people to contribute I'd want to make sure internals are type checked as well.

there's nothing to type-check internally, as Signal<T> means any value is OK as signal, and Computed<T> extends Signal works the same ... here types are for DX, tests and coverage are for guarantees ... the thing is that automatic TS creation messes up with what's internal and shouldn't bother anyone using the library, so I think I might just ditch the automatic ts generation, copy form @webreflection/signal the sync definition, enrich it with usignal extra features, and call it a day, as the API is already kinda sealed, so it shouldn't be big trouble maintaining it in the future.

Right now the automatic type export is ugly and requires patches too via bash which is extra ugly so it's clear it doesn't serve me well for this JS project.

I will update types soon but so far thanks for the PR, I might wait to have types updated before publishing again this module, if that makes sense.

Thanks!

Uhm ... actually the @webreflection/signal type is pretty different so this approach doesn't seem to be ideal ... is there any way I can enforce types via JSDoc?

I think JSDoc always takes precedence over inferred JS types, so not sure what you mean... got any examples?

current types are generated through JSDoc and yet the generator infers extra stuff it shouldn't which bothers me ... as I don't know how to tell TS via JSDoc to ignore or don't export/infer stuff from JS.

BTW, when you say missing usignal/* types what do you mean? both core and browser VS default expose types for the library ...

Could you point me to a git branch and the type you want to fix? I'll have a look if I can fix it

BTW, when you say missing usignal/* types what do you mean?

This was fixed in #20. Without it when you add / after "usignal in the import statement TS would suggest files from project root instead of files from types/*, hence import wont have correct types

the gotcha with peek() is that it's defined in a different class that is exported as if it was the Signal one, but it's not, as Signal is a base class that is extended in various places: https://github.com/WebReflection/usignal/blob/main/esm/index.js#L181

If I add manually the JSDoc in signal for something not reflected in the code, it doesn't work ... or at least I couldn't make it work.

Why do we need to hide that signal() actually returns Reactinve<T> and say it's Signal<T>?

because of the augmented methods shared with computed Reactive doesn't need nor have ... Reactive is an abstract, not a usable class.

#23 ugly, but if you don't want to say that it returns Reactive<T> then idk

That's kinda sus tho, like a red flag that something is wrong imo

point 3 is invalid/unfixable i think because (a)sync exports depend on platform and TS cannot do that