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

React-blessed types revamp #51189

Conversation

NickHackman
Copy link
Contributor

Previously all types were any now for the most part they follow blessed. Still needs a lot more work to be completely correct.

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
    Not the most meaningful, but this is a complete revamp of the previous type declarations which were very sparse.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change.
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm test <package to test>.

Addressing: issue

@Yomguithereal whenever you have the chance would love some help filling in some of the edge cases. 👍 . Still very much a work in progress, going to work on ironing out each particular component in the coming days, just wanted to have some progress shown.

Previously all types were `any` now for the most part they follow
blessed.
Added NickHackman as a maintainer of @types/react-blesssed.
Wrong version of Typescript, interfaces started with "I".
Many interface types weren't used and removed generics that made more
sense to be `unknown`.
- line
- text
- checkbox
- button
- textbox
@typescript-bot
Copy link
Contributor

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

These typings are for a version of react-blessed that doesn’t yet exist on master, so I’ve compared them with v0.3.

Comparison details 📊
0.3@master 0.7 in #51189 diff
Batch compilation
Memory usage (MiB) 121.8 124.3 +2.1%
Type count 27967 27594 -1%
Assignability cache size 6394 6206 -3%
Language service
Samples taken 138 113 -18%
Identifiers in tests 138 113 -18%
getCompletionsAtPosition
    Mean duration (ms) 586.1 649.8 +10.9%
    Mean CV 9.3% 8.5%
    Worst duration (ms) 772.7 843.1 +9.1%
    Worst identifier left content
getQuickInfoAtPosition
    Mean duration (ms) 578.4 616.2 +6.5%
    Mean CV 9.0% 8.6%
    Worst duration (ms) 767.5 783.0 +2.0%
    Worst identifier top mouse

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

@typescript-bot typescript-bot added the Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. label Feb 12, 2021
@Yomguithereal
Copy link

@NickHackman what happens if we drop the blessed- prefix? Are there some intrinsic elements that clash with the DOM?

No longer necessary that IntrinsicElements be prefixed to avoid name
collisions with core React.
@NickHackman
Copy link
Contributor Author

NickHackman commented Feb 13, 2021

@Yomguithereal from my testing locally it works perfectly fine! That's a great quality of life improvement, going to work on ironing out more of the types. I'll update this comment with the ones I think are finished.

One Question around <element> and <input>, in blessed they're both abstract classes that aren't meant to be instantiated, is there a reason why blessed-react allows them to be instantiated?

Components [Excluding Styling, because it's finicky and less clear]:

  • Box
  • Button
  • Checkbox
  • Textbox
  • Line
  • Bigtext
  • Text
  • ScrollableBox
  • ScrollableText
  • List
  • FileManager
  • ListTable
  • Listbar
  • Form
  • Textarea
  • RadioSet
  • RadioButton
  • Prompt
  • Question
  • Message
  • Loading
  • ProgressBar
  • Log
  • Table
  • Terminal
  • Image
  • ANSIImage
  • OverlayImage
  • Video
  • Layout

Added documentation to a few elements and fixed some type issues and
confusion with others.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants