Improve types

[mohd-akram]

I tried this out with a fairly complicated parser and it seems to work well. It can get pretty tricky, but that's where the tests come in.

[mohd-akram]

Any update on this?

[keichi]

Sorry for the delay, I should be able to review this over the weekend.

[mohd-akram]

Did you get a chance to look at this?

[yuhr]

I'm going to check this out soon, give me a day. Seems like it will take more time, maybe a few days, sorry for the delay.

[yuhr]

Still WIP though, I'm sure this PR must effectively be a breaking change, as the public API signatures have to be fixed to disallow invalid patterns of arguments previously allowed. I've inferred the correct signature mostly from the documentation, but I see there are some usecases that seem to be based on an undocumented behavior, so the documentation must be updated within this PR (ongoing task).

The API change tells TS to invalidate our code that concerns about invalid patterns of arguments, so I'd say we should dare to remove those parts of code (specifically runtime typechecking code) instead of introducing a bunch of @ts-expect-errors in the codebase, but it's all up to you @keichi, which way should we go for?

[mohd-akram]

I tried to keep the PR as minimal and unobtrusive as possible while getting the maximum benefit precisely to avoid going down this rabbit hole of figuring out and fixing all the types in the library, which in some ways is impossible to do due to its flexibility. Instead, relying on tests to make sure use cases are covered. My goal was to get an improvement over any in the parsed type if we can, and fall back to it if we can't to avoid breaking previous usage. It's also fairly simple to just cast to Parser<any> again to get the old behavior, or to specify a desired type. Extensive refactors and breaking changes will likely require more time and are probably best for another PR IMO, although I don't think one should bend a library just to fit within the limitations of the type system.

[yuhr]

fixing all the types in the library, which in some ways is impossible to do due to its flexibility. Instead, relying on tests to make sure use cases are covered.

Of course I don't think that the “parser alias registry” feature that radically utilizes side-effects can be fully covered by the type system, but my point is, the documentation sometimes wrongly states “It's impossible to pass it by name, just pass Parser object instead”. It's just wrong and must be updated.

My goal was to get an improvement over any in the parsed type if we can, and fall back to it if we can't to avoid breaking previous usage. It's also fairly simple to just cast to Parser<any> again to get the old behavior, or to specify a desired type. Extensive refactors and breaking changes will likely require more time and are probably best for another PR IMO, although I don't think one should bend a library just to fit within the limitations of the type system.

So the problem is backwards compatibility. I believe this PR (including the changes I commit here) doesn't require substantial changes in downstream, as long as they follow the documentation and are not abusing the previous API, but strictly speaking, every single change in static types of the public API, especially narrowing down it from any, is technically a breaking change. All we should do is just to fix as much as possible, involving fewer chances of breaking changes. If we ship a half-baked API, we will twice the chances of breaking changes.

[mohd-akram]

the documentation sometimes wrongly states “It's impossible to pass it by name, just pass Parser object instead”. It's just wrong and must be updated.

The documentation also shows it using a string in the examples, and the type in ParserOptions currently is string | Parser. I don't consider this undocumented and unsupported just because it was erroneously omitted in one instance.

So the problem is backwards compatibility. I believe this PR (including the changes I commit here) doesn't require substantial changes in downstream, as long as they follow the documentation and are not abusing the previous API, but strictly speaking, every single change in static types of the public API, especially narrowing down it from any, is technically a breaking change. All we should do is just to fix as much as possible, involving fewer chances of breaking changes. If we ship a half-baked API, we will twice the chances of breaking changes.

It is debatable whether simply narrowing the type is a breaking change. Generally, there is more leeway with type changes when it comes to semver from what I've seen in projects. TypeScript itself does not practice semver because it is difficult to distinguish between a fix and a change.

[mohd-akram]

There is no other way to get a uint16 parser without explicitly specifying endianness. This is useful in choice:

choices: {
  0: Parser.start().uint16(), // "uint16" doesn't work
  1: Parser.start().uint32(), // "uint32" doesn't work
}

[mohd-akram]

I think it's possible to create a type for this.

[devmattrick]

Sorry to bump this, but is there any progress with this PR? The DefinitelyTyped types are fairly outdated at this point so I'm wondering what the best option is to get better typings for parse results. Thanks!