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

fix(keyboard): bind-method typing still requires EventTarget #948

Merged
merged 2 commits into from
Nov 15, 2024

Conversation

torge-hmn
Copy link
Contributor

@torge-hmn torge-hmn commented Nov 11, 2024

Proposed Changes

In a previous update the ability to bind the keyboard to a specific node has been removed. The current JS-doc resulted in a generated type definition still requiring an EventTarget as parameter for the bind method even though supplying this parameter leads to an error and has no effect.

Prior (generated Keyboard.d.ts):

  /**
   * Bind keyboard events to the given DOM node.
   *
   * @param node
   */
  bind(node: EventTarget): void;

After:

  /**
   * Bind keyboard events to the given DOM node.
   *
   * @param node
   */
  bind(node: undefined): void;

Checklist

To ensure you provided everything we need to look at your PR:

  • Brief textual description of the changes present
  • Visual demo attached: See comparison above
  • Steps to try out present, i.e. using the @bpmn-io/sr tool: Just use type generation
  • Related issue linked: I've not filed an issue yet

Related to #662

@torge-hmn
Copy link
Contributor Author

I would prefer removing the parameter from the generated typing entirely if it is possible. I don't know whether the type-generation tooling has a handling for this use case, as any-typing was used in a basic attempt I tried.

@nikku
Copy link
Member

nikku commented Nov 11, 2024

CC @jarekdanielak I believe a follow-up is needed (this PR, to be checked) to properly move fix our type definitions, too.

@nikku nikku added the ready Ready to be worked on label Nov 11, 2024
@nikku
Copy link
Member

nikku commented Nov 11, 2024

Thanks @torge-hmn for the PR ⭐ . We'll have a look.

@barmac barmac requested a review from jarekdanielak November 13, 2024 10:48
@bpmn-io-tasks bpmn-io-tasks bot removed the ready Ready to be worked on label Nov 13, 2024
@nikku nikku added the needs review Review pending label Nov 14, 2024 — with bpmn-io-tasks
@@ -125,7 +125,7 @@ Keyboard.prototype._isEventIgnored = function(event) {
/**
* Bind keyboard events to the given DOM node.
*
* @param {EventTarget} node
* @param {undefined} node
*/
Keyboard.prototype.bind = function(node) {
Copy link
Member

Choose a reason for hiding this comment

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

We should rather fix the method signature, not require any argument, and check whether an argument is provided via arguments[0]. This change looks odd.

Suggested change
Keyboard.prototype.bind = function(node) {
Keyboard.prototype.bind = function() {

Copy link
Member

Choose a reason for hiding this comment

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

Fixed via 36316be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much better solution, thank you! 👍 (I wasn't sure if I could change the signature tbh)

Copy link
Member

Choose a reason for hiding this comment

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

We want to ensure that any signature change is tested; in this case I marked the old behavior as deprecated; it will throw at run-time. In the future we want to remove it all together.

@nikku nikku force-pushed the fix-keyboard-bind-typing branch from 6798456 to 36316be Compare November 14, 2024 14:17
@nikku nikku force-pushed the fix-keyboard-bind-typing branch from 36316be to 3a8434a Compare November 14, 2024 14:18
@nikku
Copy link
Member

nikku commented Nov 14, 2024

@jarekdanielak Ready for your review!

@nikku nikku assigned nikku and unassigned jarekdanielak Nov 14, 2024
@nikku nikku merged commit 0a9bb6a into bpmn-io:develop Nov 15, 2024
2 checks passed
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Nov 15, 2024
@nikku
Copy link
Member

nikku commented Nov 15, 2024

Thanks @torge-hmn for the report + initial fix.

@torge-hmn
Copy link
Contributor Author

Thanks @torge-hmn for the report + initial fix.

Thanks for fixing! 👍

@torge-hmn torge-hmn deleted the fix-keyboard-bind-typing branch November 15, 2024 10:00
@nikku
Copy link
Member

nikku commented Nov 15, 2024

Published via [email protected].

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.

3 participants