Yomguithereal / react-blessed

A react renderer for blessed.
MIT License
4.45k stars 177 forks source link

Revamping Components - Typescript Types #117

Closed NickHackman closed 3 years ago

NickHackman commented 3 years ago

I'm going to work on expanding the type definitions that were part of #86, in order to improve usability in a Typescript context. Specifically, the goal is to type all of blessed this will probably take a while, but bare with me :slightly_smiling_face:

I have a few questions on naming and conventions moving forward.

  1. The Components created by react-blessed are intrinsic types of React and therefore don't require an import, is there a reason to use this approach following other component libraries and requiring imports?

  2. Components have lowercase names? I don't believe the issue that #86 solved is still a problem, right? So we could feasibly remove the prefix of blessed- and capitalize them following React conventions?

Would greatly appreciate feedback and reviews in the future to improve the state of the project, assuming that these changes are wanted if they're not please say so :heart:

Yomguithereal commented 3 years ago

Hello @NickHackman

The Components created by react-blessed are intrinsic types of React and therefore don't require an import, is there a reason to use this approach following other component libraries and requiring imports?

react-blessed is full-fledged react custom renderer. Therefore its primitive components must be lowercased and don't require import, as if you were using blessed instead of the DOM. If you had to import components then you would need to rely on an already existing react renderer such as react-dom, which does not make sense in a shell context.

So we could feasibly remove the prefix of blessed- and capitalize them following React conventions?

If I remember correctly the possibility to add blessed- to the name was a trick to make intrinsic types work without clashing with react's own type. But capitalizing the names would be against following React conventions in a sense because you would not have any primitive components left, which make no sense. But I agree there is an issue here because you need to use React types and yet you need somehow to override react intrinsic JSX types and I have no idea how to do this.

What do you think?

NickHackman commented 3 years ago

Therefore its primitive components must be lowercased and don't require import, as if you were using blessed instead of the DOM. If you had to import components then you would need to rely on an already existing react renderer such as react-dom, which does not make sense in a shell context.

Yeah, my bad here had a brain fart. That makes sense!

But I agree there is an issue here because you need to use React types and yet you need somehow to override react intrinsic JSX types and I have no idea how to do this.

I'll look into this while I work on adding the types! Thanks for the answers, I'll link the PR in DefinitelyTyped here when I've added a few types.

OnkelTem commented 3 years ago

Folks, honestly it's really hard to understand what is going on here...

Is my understanding correct that no real TypeScript support exists for this module? Basically one doesn't simply use <box> and expect its gonna compile, right?

After searching and reading many comments I stumbled upon https://github.com/Yomguithereal/react-blessed/pull/86 PR after reading which I came to conclusion that I have to rename <box> to <blessed-box>. Fine, I did that and my project could finally compile. But.. apart from that no typing is available at all. Looking into react.d.ts just confirmed my concerns:

declare namespace JSX {
  interface IntrinsicElements {
    'blessed-box': any;
   // ...

So with any it won't work, indeed. Or am I missing something? Is it going to be any better or there are some insuperable limitations in the underlying React... contraptions?

P.S. Maybe it's worth mentioning (on this TS situation) in the README? Because it's a little frustrating to discover it after several hours of investigations.

NickHackman commented 3 years ago

@OnkelTem This issue is working on revamping the Typescript types as you found out are quite lacking to the point that they don't really do anything. The current types technically work as any works fine for components, but won't give you any autocomplete or assistance in debugging. I don't think it's necessary to update the README, and this issue is the first result when looking for "typescript"

Currently DefinitelyTyped PR exists, but I'm not actively using react-blessed and work pulling me a few different ways it's not my top priority. If you'd be willing, I think I have a decent amount of the types setup for that PR if you'd like to contribute to it further or if possible we could merge that in and continue the work elsewhere. If not it'll be done at some point in the future :+1:

OnkelTem commented 3 years ago

Thanks @NickHackman for the reply.

I've actually switched to a different framework — https://github.com/vadimdemedes/ink It's actively developed and looks more up to date (along with types).

As for blessed, it doesn't seem to be maintained as you know. That doesn't necessary signals a problem — maybe it's already ideal and there's no bugs! eh? — but that's unfortunately not the case and their issue queue has ~200 unresolved tickets.

defnotrobbie commented 3 years ago

new definitions for react-blessed were published yesterday. they use module augmentation on react and jsx to replace html intrinsic elements with react-blessed elements (with and without the blessed- prefix). I'm not sure they're 100% complete (I didn't write defs for escape and program, for example) but they cover the example code and work for a small project I used them in.