WebReflection / usignal

A blend of @preact/signals-core and solid-js basic reactivity API
ISC License
220 stars 15 forks source link

Incomplete types #19

Closed qwelias closed 1 year ago

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

for 2 see https://github.com/microsoft/TypeScript/issues/8305#issuecomment-1415874037

WebReflection commented 1 year ago

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?

qwelias commented 1 year ago

Manual vs generated types usually depends on:

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.

WebReflection commented 1 year ago

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.

qwelias commented 1 year ago

Thanks!

WebReflection commented 1 year ago

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?

qwelias commented 1 year ago

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

WebReflection commented 1 year ago

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.

WebReflection commented 1 year ago

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

qwelias commented 1 year ago

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

WebReflection commented 1 year ago

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.

qwelias commented 1 year ago

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

WebReflection commented 1 year ago

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

qwelias commented 1 year ago

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

qwelias commented 1 year ago

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