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

Annotate Events functions types with this: Core as they are always bound #10812

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

NOtherDev
Copy link

@NOtherDev NOtherDev commented Feb 23, 2024

Context

The TypeScript types exposed by Handsontable does not specify the type of this pointer for the Events methods, while they are always called by runHooks method of Core and they are always bound to Core instance. Lack of this type annotation leads to implicit any typing that is forbidden in TypeScript strict mode.

Current state:
image

State after the proposed change:
image

How has this been tested?

N/A - just a typing change

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. React wrapper: Enable TypeScript strict mode & ensure proper strong typing everywhere #10813 - WIP for TypeScript enhancements in React wrapper as discussed in RFC

Affected project(s):

  • handsontable
  • @handsontable/angular
  • @handsontable/react
  • @handsontable/vue
  • @handsontable/vue3

Checklist:

Copy link
Member

@evanSe evanSe left a comment

Choose a reason for hiding this comment

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

This is a really annoying one I think we might want to completely avoid, not all plugin hooks are called with Core as context and it is going to be a real pain to determine the context so I am fine if we need to break some typescript rules when writing our tests.

I would close the pr and make the necessary modifications on other pr if needed.

@NOtherDev
Copy link
Author

OK got it @evanSe. I'm fine closing it. We can always override at the caller side with:

init={function (this: Core) {
  mockElementDimensions(this.rootElement, 300, 300);
}}

Although note that this happens not only in the test code, but it will happen in any TS-enabled client codebase that provides the hooks via settings.

Copy link
Member

@evanSe evanSe left a comment

Choose a reason for hiding this comment

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

Actually, it is all correct 😄

@NOtherDev NOtherDev closed this Feb 26, 2024
@NOtherDev NOtherDev reopened this Feb 26, 2024
@evanSe
Copy link
Member

evanSe commented Feb 26, 2024

I will merge shortly

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

Successfully merging this pull request may close these issues.

None yet

2 participants