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

TypeScript .d.ts type definitions improvement #5767

Merged
merged 50 commits into from
Feb 18, 2019

Conversation

aaronbeall
Copy link
Contributor

@aaronbeall aaronbeall commented Feb 6, 2019

Context

Addresses: #5736
Also fixes: #5225 #5004 #5091 #5046
Includes fixes for #5719 #5721
Relates to: #5094

This is a sweeping refinement of the type definitions. Main goals were:

  • Focus on GridSettings, Hooks and Core methods (ie not plugins except where referenced by settings) to make the Handsontable config completely typed.
  • Replace ambiguous types like any and object and string (where literals are expected) with meaningful types. Basically I added a bunch of types to represent the various config objects and hook arguments.

Additional refactoring:

  • Added type aliases for common types, like CellValue, CellChange, RowObject. Intentionally left these very wide (ex CellValue = any) to avoid breaking things, but it improves readability and any future refactoring of these base types. We could potentially use type arguments to make these even tighter based on users' usage.
  • Split GridSettings up into GridSettings -> ColumnSettings -> CellMeta -> CellProperties in order to sort out some of the issues related to https://github.com/handsontable/docs/issues/12. GridSettings still has almost everything. I commented these types in the code for the rationale on this breakdown.
  • DefaultSettings is a class-like object that creates GridSettings, so I updated type annotations to reflect this and replaced all annotations of DefaultSettings to the correct type settings type (grid, column, cell).
  • Split Hooks out into Events and Hooks, where Events is just the Hooks -> Events in the docs, and Hooks the methods available on the Handsontable.hooks namespace.
  • Refined Element and Event where possible.
  • Added this types where useful, for example in validators and cells callback.
  • Added missing top-level Handsontable.validators API.

How has this been tested?

  • Added a bunch of new tests to npm run test:types, strengthened coverage using Required<> map type. Still adding some more to cover nested config objects.
  • Tested in IDE and in browser to inspect the runtime behavior the types should model.
  • Generated types using tsd-jsdoc and compared to definitions using the compiler -- found a handful of missing and wrong definitions this way, would be nice to add a script for this.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature or improvement (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Additional language file or change to the existing one (translations)

Related issue(s):

  1. Missing TypeScript configuration objects #5736
  2. Refactor Typescript definition files #5094

Checklist:

  • My code follows the code style of this project,
  • My change requires a change to the documentation.

# Conflicts:
#	handsontable.d.ts
@AMBudnik
Copy link
Contributor

AMBudnik commented Feb 8, 2019

You're really helpful. That's a great job!

@jansiegel jansiegel added this to the February 2019 milestone Feb 11, 2019
@jansiegel jansiegel self-assigned this Feb 13, 2019
@krzysztofspilka
Copy link
Member

@aaronbeall I'm deeply impressed by the amount of work you put into this.

Could you please shoot me an email at chris(at)handsontable.com? I would like to discuss one thing before we merge these changes. Thanks so much!

@jansiegel
Copy link
Member

@aaronbeall That's really impressive!

I have one small suggestion, though:

  • Split Hooks out into Events and Hooks, where Events is just the Hooks -> Events in the docs, and Hooks the methods available on the Handsontable.hooks namespace.

Internally we tend to use the term "events" as native DOM events, and "hooks" as our internal events. That being said, splitting the hook API and hooks themselves, just like you did, is definitely a good idea.

I think it would be good to move the Events interface into the Hooks namespace. This way, we avoid the confusion, as Handsontable.Hooks.Events is pretty self-explanatory.

Other than that everything looks great!

@AMBudnik
Copy link
Contributor

@wojciechczerniak awesome!

@aaronbeall
Copy link
Contributor Author

aaronbeall commented Feb 15, 2019

Internally we tend to use the term "events" as native DOM events, and "hooks" as our internal events. That being said, splitting the hook API and hooks themselves, just like you did, is definitely a good idea.

I think it would be good to move the Events interface into the Hooks namespace. This way, we avoid the confusion, as Handsontable.Hooks.Events is pretty self-explanatory.

Yep, sounds good. Done! TBH I wanted to keep the event hooks interface as Hooks and name the new top-level API namespace something else, but then it wouldn't follow the naming pattern of all the other top-level APIs.

Edit: Hmm... how about putting Hooks in Hooks.Methods?

@jansiegel
Copy link
Member

@aaronbeall

Edit: Hmm... how about putting Hooks in Hooks.Methods?

Good idea, Hooks being just a namespace would be even more consistent with our current documentation structure 👍

@aaronbeall
Copy link
Contributor Author

aaronbeall commented Feb 15, 2019

Done
(Edit: oops, clicked close by accident 😆 )

@aaronbeall aaronbeall closed this Feb 15, 2019
@aaronbeall aaronbeall reopened this Feb 15, 2019
Copy link
Member

@jansiegel jansiegel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@jansiegel jansiegel merged commit 85ccf45 into handsontable:develop Feb 18, 2019
@AMBudnik
Copy link
Contributor

AMBudnik commented Mar 6, 2019

Aaron, thanks to you we got a brand you definition file for the Handsontable 7.0.0 released today.

Thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants