Skip to content

fix(useOrientation): correct the inverted isBrowser guards in lockOrientation/unlockOrientation - #215

Merged
childrentime merged 1 commit into
childrentime:mainfrom
ostapondo:fix/orientation-lock-inverted-guard
Jul 30, 2026
Merged

fix(useOrientation): correct the inverted isBrowser guards in lockOrientation/unlockOrientation#215
childrentime merged 1 commit into
childrentime:mainfrom
ostapondo:fix/orientation-lock-inverted-guard

Conversation

@ostapondo

Copy link
Copy Markdown
Contributor

Description

Follow-up to the note in #211: the isBrowser guards in useOrientation's
lockOrientation / unlockOrientation are inverted:

if (isBrowser) {
  return
}

So both functions no-op in every browser — screen.orientation.lock() /
unlock() are never called — and fall through on the server, where the bare
window reference on the next line throws ReferenceError: window is not defined. Neither path had test coverage.

Fix: if (!isBrowser) return. The unsupported-browser rejection and the
promise passthrough from lock() are unchanged — they were just unreachable
before.

One test-infra change: jest-setup.ts touched window unconditionally, so a
@jest-environment node suite couldn't run at all. The setup is now guarded
with typeof window !== 'undefined'; jsdom suites are unaffected.

Type of Change

  • Bug fix
  • New hook
  • Enhancement to existing hook
  • Documentation update
  • Other (please describe)

Checklist

  • I have read the Contributing Guide
  • My code follows the project's coding style
  • I have added tests for my changes
  • All existing tests pass
  • I have updated the documentation

No doc change — the docs already describe the fixed behaviour.

Five tests added, four of them fail on main:

  • lockOrientation calls screen.orientation.lock(type) and returns its promise
  • unlockOrientation calls screen.orientation.unlock()
  • lockOrientation rejects with Not supported when screen.orientation is absent
  • an SSR spec (@jest-environment node, renderToString) — fails on main with the ReferenceError

The fifth (unlockOrientation doesn't throw without screen.orientation) also
passes on main; it covers the support branch this fix makes reachable.

Full suite passes (312 tests). Also verified against the built dist/index.mjs
in plain Node: render + lock()/unlock() are no-ops, no throw.

Not covered: browsers where screen.orientation exists but lock doesn't
(iOS Safari) still throw a sync TypeError rather than reject — same support
check as before.

#211 touches the same file, so whichever lands second may need a trivial rebase.

@childrentime
childrentime merged commit dba5520 into childrentime:main Jul 30, 2026
4 checks passed
@childrentime

Copy link
Copy Markdown
Owner

Thanks @ostapondo! Great catch on the inverted guards — both call paths were broken and nothing in the suite noticed. Really appreciate that the tests were written to fail on main first, and the jest-setup.ts guard is a nice bonus that unblocks plain node-environment SSR specs for the whole repo. Merged 🎉

childrentime added a commit that referenced this pull request Jul 31, 2026
Fixes
- useOrientation: lockOrientation/unlockOrientation had inverted isBrowser
  guards — they early-returned in the browser and only ran during SSR,
  making both no-ops where they mattered. Closes #215.
- useInterval: with controls: true, an interval started through resume()
  kept firing after unmount, and calling resume() twice leaked a timer
  neither pause() nor unmount could reach. Closes #212.
- useMicrophone: level stayed frozen at the last reading after stop();
  it now resets to 0. Closes #213.
- useElementByPoint: multiple mode re-rendered on every rAF frame because
  elementsFromPoint allocates a fresh array; the hit list is now compared
  element-by-element and kept stable when unchanged. Closes #214.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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