logux / client

Logux base components to build web client
https://logux.org/
MIT License
663 stars 47 forks source link

Fix StatusListener typing #98

Closed mifopen closed 2 years ago

mifopen commented 2 years ago

Fixed details type for syncError type and made it more inferring-friendly

ai commented 2 years ago

Will it remove the argument names from the autocomplete?

Can we define a multiple interfaces instead?

Like:

interface StatusListener {
  ('synchronized'): void
  ('syncError', { error: Error })
  …
}
mifopen commented 2 years ago

Will it remove the argument names from the autocomplete?

No.

Can we define a multiple interfaces instead?

It doesn't work with inferring. So you won't be able to write

status(store.client, (state, details) => {
  switch(status) {
    case 'syncError':
      console.log(details.error); // <- here details is inferred as { error: Error }
    // ... <- here you can use your IDE to generate all the branches as state is correct union type
});
ai commented 2 years ago

It doesn't work with inferring

Got it.

Can you fix a type test and we can release it.

mifopen commented 2 years ago

On it

mifopen commented 2 years ago

The only downside (I guess because of the amateur TS support of the feature) is that you can not pass a callback with one parameter in the signature — it must be the full tuple. Had do ignore eslint warning about unused argument in test

ai commented 2 years ago

The only downside (I guess because of the amateur TS support of the feature) is that you can not pass a callback with one parameter in the signature

Not a solution

mifopen commented 2 years ago

The only downside (I guess because of the amateur TS support of the feature) is that you can not pass a callback with one parameter in the signature

Not a solution

Could you please elaborate on what you mean by that? :)

mifopen commented 2 years ago

I guess it's backwards incompatible. Ok, I see

ai commented 2 years ago

I guess it's backwards incompatible. Ok, I see

Yes. But also it is not useful.

mifopen commented 2 years ago

I guess it's backwards incompatible. Ok, I see

Yes. But also it is not useful.

Besides backward incompatibility, I don't see any downsides. It's definitely better than it was because you don't have to check the structure of the details before accessing it.

mifopen commented 2 years ago

Will try to fix it somehow

ai commented 2 years ago

Besides backward incompatibility, I don't see any downsides.

You need to disable linter if you need only single argument

mifopen commented 2 years ago

Yeah, I know. That's what I'm trying to deal with

mifopen commented 2 years ago

Ok, the current approach is the recommended one by TS https://devblogs.microsoft.com/typescript/announcing-typescript-4-6/#control-flow-analysis-for-dependent-parameters And there is no workaround for not declaring the second parameter in this case. And of course, the language does nothing with the fact that someone wants to use such lint rules. So for a better solution here we have to wait for https://github.com/microsoft/TypeScript/issues/22609. I will close the PR and patch the package locally for us as we always use details parameter. Sorry for bothering you.

mifopen commented 2 years ago

aeh, there is a bug so let's fix it at least :)

ai commented 2 years ago

Thanks. Released in 0.18.4.