Skip to content

Feat svelte adapter#45

Merged
KevinVandy merged 18 commits intoTanStack:mainfrom
kunaaal13:feat-svelte-adapter
Mar 10, 2026
Merged

Feat svelte adapter#45
KevinVandy merged 18 commits intoTanStack:mainfrom
kunaaal13:feat-svelte-adapter

Conversation

@kunaaal13
Copy link
Contributor

@kunaaal13 kunaaal13 commented Feb 25, 2026

🎯 Changes

✅ Checklist

  • I have followed the steps in the Contributing guide.
  • I have tested this code locally with pnpm run test:pr.

🚀 Release Impact

  • This change affects published code, and I have generated a changeset.
  • This change is docs/CI/dev-only (no release).

@kunaaal13 kunaaal13 marked this pull request as draft February 25, 2026 09:29
@kunaaal13
Copy link
Contributor Author

@KevinVandy have added examples as well. What is the process now.

@KevinVandy
Copy link
Member

I just have to have enough time to review

@KevinVandy KevinVandy marked this pull request as ready for review March 8, 2026 18:30
@kunaaal13
Copy link
Contributor Author

@KevinVandy how does it look? can i help with something?

@KevinVandy
Copy link
Member

I'm doing a deeper review and testing now. First pass was just getting all the tests to pass

@pkg-pr-new
Copy link

pkg-pr-new bot commented Mar 8, 2026

Open in StackBlitz

@tanstack/angular-hotkeys

npm i https://pkg.pr.new/TanStack/hotkeys/@tanstack/angular-hotkeys@45

@tanstack/hotkeys

npm i https://pkg.pr.new/TanStack/hotkeys/@tanstack/hotkeys@45

@tanstack/hotkeys-devtools

npm i https://pkg.pr.new/TanStack/hotkeys/@tanstack/hotkeys-devtools@45

@tanstack/preact-hotkeys

npm i https://pkg.pr.new/TanStack/hotkeys/@tanstack/preact-hotkeys@45

@tanstack/preact-hotkeys-devtools

npm i https://pkg.pr.new/TanStack/hotkeys/@tanstack/preact-hotkeys-devtools@45

@tanstack/react-hotkeys

npm i https://pkg.pr.new/TanStack/hotkeys/@tanstack/react-hotkeys@45

@tanstack/react-hotkeys-devtools

npm i https://pkg.pr.new/TanStack/hotkeys/@tanstack/react-hotkeys-devtools@45

@tanstack/solid-hotkeys

npm i https://pkg.pr.new/TanStack/hotkeys/@tanstack/solid-hotkeys@45

@tanstack/solid-hotkeys-devtools

npm i https://pkg.pr.new/TanStack/hotkeys/@tanstack/solid-hotkeys-devtools@45

@tanstack/svelte-hotkeys

npm i https://pkg.pr.new/TanStack/hotkeys/@tanstack/svelte-hotkeys@45

@tanstack/vue-hotkeys

npm i https://pkg.pr.new/TanStack/hotkeys/@tanstack/vue-hotkeys@45

@tanstack/vue-hotkeys-devtools

npm i https://pkg.pr.new/TanStack/hotkeys/@tanstack/vue-hotkeys-devtools@45

commit: e153132

Copy link

@paoloricciuti paoloricciuti left a comment

Choose a reason for hiding this comment

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

Took a look at the code part and there are a few confusing things...if the code is working either there's probably some edge case missing or the code can be simplified a lot while continuing to work.

If you need any help feel free to ping me!

Comment on lines +69 to +80
* let ref = $state<HTMLDivElement | null>(null)
* let count = $state(0)
*
* createHotkey('Mod+S', () => {
* console.log('Mod+S pressed')
* count++
* }, { target: ref })
* </script>
*
* <div bind:this={ref}>
* Count: {count}
* </div>

Choose a reason for hiding this comment

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

Rather than doing this I would create an attachment (docs: https://svelte.dev/docs/svelte/@attach) that can retrieve a reference automatically from the template without using an extra variable.

Copy link

Choose a reason for hiding this comment

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

Probably best as its own function since global hotkeys are a common use case.

const increment = createHotkeyAttachment('Mod+S', () => {
    console.log('Mod+S pressed')
    count++
});
<div {@attach increment}>
   Count: {count}
</div>

Copy link

Choose a reason for hiding this comment

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

Or maybe: Always return an attachment/an object that can create an attachment and make it possible to disable the global nature of the hotkey. I.e.

const increment = createHotkey('Mod+S', () => {
    console.log('Mod+S pressed')
    count++
}, { target: null });

Comment on lines +95 to +101
let callbackRef = callback
let managerRef = manager

$effect(() => {
callbackRef = callback
managerRef = manager
})

Choose a reason for hiding this comment

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

This feels weird to me: callback and manager are not stateful variables so this $effect will never re-trigger no?

? document
: null

// Skip if no valid target (SSR or ref still null)

Choose a reason for hiding this comment

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

$effect doesn't run during SSR so if you were architecting around SSR maybe something can be simplified?

* </script>
*
* <div>
* <button on:click={recorder.startRecording}>

Choose a reason for hiding this comment

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

Suggested change
* <button on:click={recorder.startRecording}>
* <button onclick={recorder.startRecording}>

Comment on lines +50 to +52
* {recorder.recordedHotkey && (
* <div>Recording: {recorder.recordedHotkey}</div>
* )}

Choose a reason for hiding this comment

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

Suggested change
* {recorder.recordedHotkey && (
* <div>Recording: {recorder.recordedHotkey}</div>
* )}
* {#if recorder.recordedHotkey}
* <div>Recording: {recorder.recordedHotkey}</div>
* {/if}

Comment on lines +60 to +71
const mergedOptions = {
...getDefaultHotkeysOptions().hotkeyRecorder,
...options,
} as HotkeyRecorderOptions

const recorder = new HotkeyRecorder(mergedOptions)

// Sync options on every render (same pattern as createHotkey)
// This ensures callbacks always have access to latest values
$effect(() => {
recorder.setOptions(mergedOptions)
})

Choose a reason for hiding this comment

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

mergedOptions is not a stateful variable so this effect will always run once...we should probably make mergedOptions a derived (but then you should pay attention because accepting an object as argument instead of a getter means the user will not be able to reassign the whole options reactively but just mutate them)...I would probably switch every option to be a a getter.

Copy link

Choose a reason for hiding this comment

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

If the options were not spread, the individual properties could already be made reactive via get prop() { return ... } at the call site.

For APIs like this it can also be convenient to have the function take either a callback returning an options object or an options object directly. That way you do not necessarily need many smaller functions or get properties to have something reactive.

Forcing each property to be a callback makes the calling code a bit noisy, but you could also potentially accept T | (() => T) on each, depending on how much overloading one is comfortable with, though this will clash on properties that are already callbacks...

Comment on lines +82 to +89
// Refs to capture current values for use in effect without adding dependencies
let callbackRef = callback
let optionsRef = mergedOptions

$effect(() => {
callbackRef = callback
optionsRef = mergedOptions
})

Choose a reason for hiding this comment

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

Same as above

Copy link

Choose a reason for hiding this comment

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

Bit of syntax chaos here.

  • Old store syntax on $heldKeys.
  • What looks like broken reactivity on isShiftHeld (neither .current, nor $)
  • class:active={...} could be replaced with the newer clsx object syntax: class={{ active: ... }}

Comment on lines +60 to +71
const mergedOptions = {
...getDefaultHotkeysOptions().hotkeyRecorder,
...options,
} as HotkeyRecorderOptions

const recorder = new HotkeyRecorder(mergedOptions)

// Sync options on every render (same pattern as createHotkey)
// This ensures callbacks always have access to latest values
$effect(() => {
recorder.setOptions(mergedOptions)
})
Copy link

Choose a reason for hiding this comment

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

If the options were not spread, the individual properties could already be made reactive via get prop() { return ... } at the call site.

For APIs like this it can also be convenient to have the function take either a callback returning an options object or an options object directly. That way you do not necessarily need many smaller functions or get properties to have something reactive.

Forcing each property to be a callback makes the calling code a bit noisy, but you could also potentially accept T | (() => T) on each, depending on how much overloading one is comfortable with, though this will clash on properties that are already callbacks...

import { HotkeysProvider } from '@tanstack/svelte-hotkeys'
</script>

<HotkeysProvider
Copy link

Choose a reason for hiding this comment

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

I would promote the use of setHotkeysContext over this. The component is essentially pointless unless you have multiple providers for different parts of the application at the same time.

Just call the function in the script, save one level of nesting.

@kunaaal13
Copy link
Contributor Author

@KevinVandy i am up for the changes, will push the fixes as per discussion above in some hours?

@KevinVandy
Copy link
Member

@kunaaal13 Heads up that I'm pushing a major refactor here in a little bit too.

@KevinVandy
Copy link
Member

@paoloricciuti @brunnerh I did a big refactor based on your feedback. I think the attachment stuff works a lot better, but had to keep a separate createHotkeyAttachment function to simplify the default document/window one.

Also, I just competely got rid of @tanstack/svelte-store for now since the simple state getter wrappers seem to work better. I might bring back tanstack store in more correct way later.

@KevinVandy KevinVandy merged commit 75ce7c8 into TanStack:main Mar 10, 2026
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.

4 participants