Skip to content

Conversation

@braden-w
Copy link
Contributor

@braden-w braden-w commented Jun 19, 2025

This PR changes the default behavior of the PressedKeys utility to use exact key matching instead of inclusive matching. This means has("k") will now only return true when ONLY the "k" key is pressed, not when "Cmd+K" is pressed.

Problem

The previous behavior was counterintuitive and didn't align with user expectations or industry best practices:

// Previous behavior - confusing!
// When Cmd+K is pressed:
keys.has("k") // returned true 
keys.has("meta", "k") // also returned true

This made it impossible to distinguish between a single key press and a key combination, leading to unexpected behavior in applications.

Solution Options Considered

Option 1: Add options parameter (backward compatible)

keys.has("k", { exact: true }) // Only "k" alone

Option 2: Add new methods

keys.hasExact("k") // Only "k" alone

Option 3: Change default behavior (breaking change) ✅

Make exact matching the default and add an option for inclusive matching.

Implementation (Option 3 - Simplified)

I implemented Option 3 with a simplified approach - exact matching is now the only behavior:

// New behavior - intuitive!
// When only "k" is pressed:
keys.has("k") // true
keys.has("k", "meta") // false

// When Cmd+K is pressed:
keys.has("k") // false (exact match)
keys.has("k", "meta") // true (exact match)

Changes Made

  1. Updated has method to use exact matching only
  2. Updated onKeys method to use exact matching only
  3. Added comprehensive tests for exact matching behavior
  4. Updated documentation with clear examples of exact matching

Breaking Changes

⚠️ This is a breaking change that will affect existing code:

  • has("k") will no longer match when modifier keys are also pressed
  • onKeys("k", callback) will no longer fire when modifier keys are also pressed

Migration Guide

The previous inclusive behavior can be achieved using keys.all.includes():

// Before (inclusive behavior)
keys.has("k") // true when "k" OR "Cmd+k" pressed

// After - Option 1: Exact matching (new default)
keys.has("k") // true only when "k" alone is pressed

// After - Option 2: Inclusive behavior (use .all.includes())
keys.all.includes("k") // true when "k" is pressed with or without modifiers

This gives users a clear migration path - they can simply replace keys.has("k") with keys.all.includes("k") to maintain the old behavior.

Benefits

  1. More intuitive API: Single key checks behave as expected
  2. Better distinction: Clear difference between "k" and "Cmd+K"
  3. Industry alignment: Matches behavior of popular libraries like Mousetrap
  4. Simpler API: One clear behavior without confusing options

Discussion

While I implemented Option 3 due to its superior ergonomics, this decision is open for discussion. The final implementation removes the inclusive option entirely to maintain a simpler, more focused API.

If users need to check whether a key is pressed regardless of modifiers, they can use keys.all.includes("k") directly.

I believe the benefits of the more intuitive API outweigh the breaking change, and the simplified implementation (without the inclusive option) makes the behavior clear and predictable.

@changeset-bot
Copy link

changeset-bot bot commented Jun 19, 2025

⚠️ No Changeset found

Latest commit: 590fdbb

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@huntabyte
Copy link
Member

I think this makes a lot of sense. @TGlide since this is your brain child, what do you think?

@github-actions
Copy link
Contributor

github-actions bot commented Jun 20, 2025

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name Status Preview Last Commit
runed ✅ Ready (View Log) Visit Preview 590fdbb

@TGlide
Copy link
Member

TGlide commented Jul 6, 2025

Sorry for the huge wait! let me check

@TGlide
Copy link
Member

TGlide commented Jul 6, 2025

I propose a different solution. A mix of 1 and something else.

const keys = new PressedKeys({ exact: true })
// When only "k" is pressed:
keys.has("k") // true
keys.has("k", "meta") // false

// When Cmd+K is pressed:
keys.has("k") // false (exact match)
keys.has("k", "meta") // true (exact match)


// Or... Old behaviour
const keys = new PressedKeys()
// When only "k" is pressed:
keys.has("k") // true
keys.has("k", "meta") // false

// When Cmd+K is pressed:
keys.has("k") // true
keys.has("k", { exact: true }) // true
keys.has("k", "meta") // true (exact match)

I do find it more intuitive that keys.has("k") represents that k is pressed no matter what. But I can see the value for exact matching!

@JoBurgard
Copy link

JoBurgard commented Jul 12, 2025

For me a version of Option 2 makes the most sense.
I suspect that when creating hotkeys for an app, exact and non-exact matching are both useful.
Sometimes hotkey actions build on each other (additive).

Having two functions makes them equally accessible.

const keys = new PressedKeys()

if (keys.matchOne("ArrowDown", "j")) {
  // Cursor down
}

if (keys.matchOne("ArrowUp", "k")) {
  // Cursor up
}

if (keys.match("Control", "z")) {
  // Undo
}

if (keys.has("w") {
  let speedZ = 1;
  if (keys.has("Shift") {
    // sneak
    speedZ = 0.5;
  }
  // move forward
}

Maybe the version with const keys = new PressedKeys({ exact: true }) would be hard to reason about when not seeing this part of the code and the rest says keys.has(...). It would be easy to mistake it for the non-exact version of keys.has().

// Edit: improved examples

@TGlide
Copy link
Member

TGlide commented Jul 14, 2025

I wouldn't mind option 2 either, tbh!

@huntabyte
Copy link
Member

100% agree the current behavior is counterintuitive. However, I'd ALSO like to propose a variation of Option 2 that I think gives us the best of all worlds:

keys.hasAny("k") // true if k is pressed (with or without modifiers)
keys.hasExact("k") // true only if k alone is pressed
keys.hasExact("meta", "z") // true only if cmd+z is pressed
keys.hasOne("j", "ArrowDown") // true if ANY of these keys are pressed

I feel comfortable breaking keys.has because it is not intuitive at all how it works and everyone will thank themselves for updating now. You shouldn't need to look at comments or docs to understand the behavior.

When someone reads keys.hasExact("k") six months from now, the intent is immediately clear without needing to remember any options or check docs. Each method has a clear, single responsibility.

Explicit > implicit in this case.

@braden-w
Copy link
Contributor Author

@huntabyte +1, really like the explicitness of hasAny and hasExact!

@huntabyte
Copy link
Member

Now that I think more about it, we'd probably also need like hasCombination(['ctrl', 'z'], ['meta', 'z']) ?

@braden-w
Copy link
Contributor Author

braden-w commented Sep 27, 2025

Wouldn't that be covered by hasExact('ctrl', 'z') || hasExact('meta', 'z')? Although from a developer experience perspective, it might be nice to have hasCombination.

Also, was wondering if we could consider the semantics being keys.hasExact(["meta", "z"]) instead of keys.hasExact("meta", "z"). I feel like one parameter in the function that is explictly an array rather than spread would also be more explicit.

@TGlide
Copy link
Member

TGlide commented Oct 13, 2025

Explicit > implicit in this case.

Agreed!

Now that I think more about it, we'd probably also need like hasCombination(['ctrl', 'z'], ['meta', 'z']) ?

Not sure to be honest. As @braden-w said, a || seems enough, no? Also what if you wanted the behaviour of hasAny(...) || hasExact(...)?

I feel like one parameter in the function that is explictly an array rather than spread would also be more explicit.

Agreed!

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