Skip to content

Fix/type overload and dom containment - #245

Merged
yoannmoinet merged 2 commits into
yoannmoinet:masterfrom
akromann:fix/type-overload-and-dom-containment
May 13, 2026
Merged

Fix/type overload and dom containment#245
yoannmoinet merged 2 commits into
yoannmoinet:masterfrom
akromann:fix/type-overload-and-dom-containment

Conversation

@akromann

Copy link
Copy Markdown
Contributor

Hi!

Thanks so much for the rewrite to TypeScript, it will be much nicer to use now!

Unfortunately when trying to use the new 1.0 release, I came across a few issues:

  1. Multiple managers could not each open a joystick at the same time.
  2. The type information for the 'on' event handlers were not working, forcing me to cast it as any.

I built local patches for these and tested these on my production build, but it would of course be preferable to get these issues fixed upstream as well.

akromann added 2 commits May 13, 2026 13:28
…tion

SuperEventType<T> was defined as `${T}${string}` | `${string}${T}`.
While a plain string literal like 'added' is logically assignable to
`added${string}` (via empty suffix), TypeScript's overload resolution
fails to select the correct overload when multiple overloads use similar
broad template literal patterns with 19+ overloads.

Adding T explicitly ensures TypeScript can unambiguously match a plain
event name to the correct overload, inferring the callback data type
without requiring a cast to `any`.
addToDom() and removeFromDom() used document.body.contains() to check
whether the joystick element was already in the DOM. This breaks when
the zone element is not inside document.body, such as in a Shadow DOM
or a detached container (e.g. Angular components with encapsulation).

Replace with zone.contains() so the check is scoped to the actual zone
where the joystick is managed.
@akromann

Copy link
Copy Markdown
Contributor Author

Please let me know if you need me to split this PR into two or if any other edits need to be made.

@yoannmoinet yoannmoinet left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks a ton for your PR.
This looks good, no need to split it or anything.
Well done!

@yoannmoinet
yoannmoinet merged commit e60897d into yoannmoinet:master May 13, 2026
3 checks passed
@akromann

Copy link
Copy Markdown
Contributor Author

No problem, thanks for merging!
Any idea when a release will be made with this fix in?

@yoannmoinet

Copy link
Copy Markdown
Owner

I'll try and publish it later today.

@akromann

Copy link
Copy Markdown
Contributor Author

Beautiful, thanks.

@yoannmoinet

Copy link
Copy Markdown
Owner

This is now released in v1.0.3.
Thanks a ton for your contribution, this is super appreciated.

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.

2 participants