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

Incomplete types #19

Closed
qwelias opened this issue Feb 11, 2023 · 15 comments
Closed

Incomplete types #19

qwelias opened this issue Feb 11, 2023 · 15 comments

Comments

@qwelias
Copy link
Contributor

qwelias commented Feb 11, 2023

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

qwelias commented Feb 11, 2023

for 2 see microsoft/TypeScript#8305 (comment)

@WebReflection
Copy link
Owner

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
Copy link
Contributor Author

qwelias commented Feb 11, 2023

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.

@WebReflection
Copy link
Owner

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
Copy link
Contributor Author

qwelias commented Feb 11, 2023

Thanks!

@WebReflection
Copy link
Owner

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
Copy link
Contributor Author

qwelias commented Feb 13, 2023

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

@WebReflection
Copy link
Owner

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
Copy link
Owner

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

@qwelias
Copy link
Contributor Author

qwelias commented Feb 13, 2023

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
Copy link
Owner

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
Copy link
Contributor Author

qwelias commented Feb 13, 2023

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

@WebReflection
Copy link
Owner

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

@qwelias
Copy link
Contributor Author

qwelias commented Feb 13, 2023

#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
Copy link
Contributor Author

qwelias commented Feb 14, 2023

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

@qwelias qwelias closed this as completed Feb 14, 2023
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

2 participants