Skip to content

chore: use arrow functions in FoleyHandle interface - #6

Merged
eakbulut merged 1 commit into
eakbulut:mainfrom
DerTimonius:patch-1
Aug 19, 2026
Merged

chore: use arrow functions in FoleyHandle interface#6
eakbulut merged 1 commit into
eakbulut:mainfrom
DerTimonius:patch-1

Conversation

@DerTimonius

Copy link
Copy Markdown
Contributor

Thanks for this awesome package, planning to add it everywhere our CTO allows me to!

Description

I noticed that in React/TS projects with certain eslint rules (specifically the typescript/unbound-method rule), there is an error associated with the functions and possible lost this scope.

Checking the type definition of the FoleyHandle interface in the react package, I noticed that every function was defined as a named function declaration. They handle the this scope a bit differently than arrow functions, hence the lint error.

I checked if the actual code of @foley/core is using the this scope in any of the functions. Since they are not, the this binding is irrelevant.
Defining the types then as arrow functions tells TypeScript (and the linter) that these functions are saved to be detached, destructured etc.

Runtime Implications

This does not have any runtime implications, as the functions don't use this in their execution.

@eakbulut
eakbulut merged commit de42b80 into eakbulut:main Aug 19, 2026
1 check passed
eakbulut added a commit that referenced this pull request Aug 19, 2026
PR #6 reported a real type defect — method-shorthand signatures make
`const { play } = useFoley()`, the pattern the README documents, trip
typescript-eslint's unbound-method rule. Nothing in this repo could have
caught it: every existing type guard greps the d.ts to check declarations
EXIST, never that they are correct, and typescript was not even installed.

Two guards, because they catch different failures:

- tsc --strict over types/consumer.ts proves the types work for real usage.
  Its @ts-expect-error lines must keep erroring, so widening anything to
  `any` fails the build — a d.ts of `any` would pass a compile check alone.
- a shape guard forbidding method shorthand in any d.ts interface. tsc
  cannot see this one: both forms compile identically, only the property
  form is safe to detach.

Both were mutation-tested. Restoring `stop(): void` fails the shape guard;
widening the React handle's play to `any` fails the compile guard. The
second mutation initially passed, which exposed that the fixture only
exercised the core play — fixed by adding negatives for the React handle.

Applied the same fix to PlayHandle.stop in core, which had the identical
problem one package over.

The fixture found a documentation bug on its first run: the README's
`mySound[2].f = 880` does not compile, since Spec is a union and cluster
layers carry fMin/fMax. Added the narrowing pattern to the README rather
than loosening the types.
eakbulut added a commit that referenced this pull request Aug 19, 2026
Interface members are properties rather than method shorthand, so the
destructuring the docs recommend stops tripping unbound-method. Reported
for the React package by @DerTimonius in #6; PlayHandle.stop had it too.

Type-only, no runtime change — the bundled size is unchanged at 7.2 kB.
@eakbulut

eakbulut commented Aug 19, 2026

Copy link
Copy Markdown
Owner

@DerTimonius Thank you! This is a good catch, and the writeup made it easy to verify.

I checked that useFoley returns bare module-function references and that nothing in the core touches this, so detaching is safe, and I confirmed your change preserves strictness rather than just silencing the rule, an intersection type is easy to get subtly wrong, and yours resolves the on overloads exactly as before.

The part that stung was that this repo couldn't have caught it. Every type guard here greps the .d.ts to check declarations exist, never that they're correct, and typescript wasn't even a devDependency; I had to run tsc by hand to review your PR. So I've merged this and followed up in #7 with a tsc --strict fixture plus a guard that forbids method shorthand in any .d.ts interface, since tsc can't see that difference on its own. PlayHandle.stop in the core had the identical problem, now fixed too.

Both shipped in 2.8.1. And please let me know how I can improve foley as you start using it in real-world scenarios!

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