Skip to content

feat: add use-arrow-clusters react hook package - #8

Merged
StoneTapeStudios merged 8 commits into
StoneTapeStudios:mainfrom
aysead:feat/use-arrow-clusters-hook
Apr 16, 2026
Merged

feat: add use-arrow-clusters react hook package#8
StoneTapeStudios merged 8 commits into
StoneTapeStudios:mainfrom
aysead:feat/use-arrow-clusters-hook

Conversation

@aysead

@aysead aysead commented Mar 5, 2026

Copy link
Copy Markdown

Hi @StoneTapeStudios,

As discussed, I've created the initial structure for the use-arrow-clusters hook as a separate package to keep the core framework-agnostic.

What's included in this draft:
Scaffolded a new "use-arrow-clusters" package within the monorepo ("package.json", "tsdown", "tsconfig").
Implemented the "useArrowClusters" hook with the dual "useMemo" approach we talked about:

  1. Expensive operation: "engine.load()" only runs when the "table" reference or core "options" change.
  2. Cheap operation: "engine.getClusters()" runs when the map viewport ("bounds" / "zoom") updates.

Next steps to discuss:
Before I add tests and update the README with the usage examples (and the note about Arrow Table reference stability), I wanted to get your feedback on this initial structure and the hook's API signature.

Let me know if this direction looks good to you!

Comment thread packages/use-arrow-clusters/src/index.ts Outdated
Comment thread packages/use-arrow-clusters/src/index.ts Outdated
Comment thread packages/use-arrow-clusters/src/index.ts Outdated
Comment thread packages/use-arrow-clusters/src/index.ts Outdated
Comment thread packages/use-arrow-clusters/src/index.ts
@StoneTapeStudios

StoneTapeStudios commented Mar 9, 2026

Copy link
Copy Markdown
Owner

@aysead Great start, left some feedback on the memo deps and types — let me know what you think

@aysead

aysead commented Mar 23, 2026

Copy link
Copy Markdown
Author

Hi @StoneTapeStudios,
Thank you for the detailed review and the great catch on the useMemo dependencies. I've updated the dependency arrays to prevent those unnecessary re-calculations. I also applied the other suggestions:

  • Reused ArrowClusterEngineOptions from the main package.
  • Exposed idColumn as an optional parameter.
  • Exported the UseArrowClustersResult interface.
    I just pushed the updated commits. Thanks again for the guidance.

@aysead
aysead marked this pull request as ready for review March 23, 2026 12:03
@aysead
aysead marked this pull request as draft March 23, 2026 12:06
@aysead
aysead marked this pull request as ready for review March 23, 2026 12:07
@aysead

aysead commented Mar 30, 2026

Copy link
Copy Markdown
Author

Hi @StoneTapeStudios, I've just pushed the missing package.json and pnpm-lock.yaml files so the CI can find the test dependencies. It looks like the GitHub Actions workflow is waiting for a maintainer's approval to run ('Action required'). Could you please approve the workflow run when you have a chance? Thanks again for all the guidance.

@StoneTapeStudios StoneTapeStudios 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.

@aysead Nice work on this iteration — the hook structure is clean and the dependency array improvements from the last round are solid. Left another round of feedback, mostly around tightening up the tests and a few small consistency items.

The main ones:

  • The test mock should return a proper ClusterOutput shape instead of [] — the other packages' tests have a makeOutput() pattern worth following here
  • Missing a memoization stability test (verifying the engine isn't re-created when only bounds/zoom change), which is the core thing this hook is supposed to guarantee
  • A few small housekeeping items: missing test script, quote consistency

Also left a note on the useMemo + engine.load() tradeoff — not a blocker, just something to be aware of given how heavy load() is compared to the vanilla Supercluster equivalent.

Let me know if any of the comments need clarification.

Comment thread packages/use-arrow-clusters/tests/useArrowClusters.test.ts
Comment thread packages/use-arrow-clusters/src/index.ts Outdated
Comment thread packages/use-arrow-clusters/package.json Outdated
Comment thread vitest.workspace.ts Outdated
Comment thread packages/use-arrow-clusters/README.md Outdated
Comment thread packages/use-arrow-clusters/tests/useArrowClusters.test.ts
Comment thread packages/use-arrow-clusters/src/index.ts
Ayşe Asena DAĞCI added 2 commits April 7, 2026 08:54
docs(use-arrow-clusters): update wording and add filterMask warning
@aysead

aysead commented Apr 7, 2026

Copy link
Copy Markdown
Author

Hi @StoneTapeStudios,

I've just pushed the final updates:

Updated the test mock to return a proper ClusterOutput shape using a makeOutput() helper.

Added the missing memoization stability test to verify the engine isn't re-created unnecessarily.

Refactored the hook to use the useRef manual memoization pattern for safer engine stability, as you suggested.

Fixed the minor housekeeping items (test script in package.json, quote consistency, and the README wording).

Ready for your review.

@StoneTapeStudios
StoneTapeStudios merged commit b5de557 into StoneTapeStudios:main Apr 16, 2026
1 check passed
@StoneTapeStudios

Copy link
Copy Markdown
Owner

Hey @aysead ,

Everything looks great! Thanks so much for your work on this. Eventually it'd be nice if we had a React example in the examples folder.

I'd be interested to see/hear how you're using the package.

All the best 🙏

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