Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(demo): fix delaunay-voronoi demo #1758

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix(demo): fix delaunay-voronoi demo #1758

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1462a22
fix(visx-demo): fix setHoveredId in delaunay-voronoi
kelvin27315 Oct 18, 2023
0d67dcd
fix(visx-demo): fix invalid bounds in delaunay-voronoi
kelvin27315 Oct 18, 2023
fb47cc7
Refactor to avoid early return and adhere to hooks rules
kelvin27315 Oct 20, 2023
07a06d7
Merge branch 'airbnb:master' into fix-voronoi-demo
kelvin27315 Nov 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/visx-demo/src/sandboxes/visx-delaunay-voronoi/Example.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,9 @@
const innerWidth = width - margin.left - margin.right;
const innerHeight = height - margin.top - margin.bottom;

if (innerWidth < 0 || innerHeight < 0) return null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

an early return statement will break the rules of hooks, where they would then be called conditionally/a different # of times per render.

maybe a different approach (since we already return null if width < 10), is to add something like const innerWidth = Math.max(0, width - margin.left - margin.right)

Copy link
Author

Choose a reason for hiding this comment

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

I completely forgot about the rules of hooks. 😅
I've pushed a commit reflecting your comment here. Thank you!


const voronoiDiagram = useMemo(

Check failure on line 42 in packages/visx-demo/src/sandboxes/visx-delaunay-voronoi/Example.tsx

View workflow job for this annotation

GitHub Actions / build 

React Hook "useMemo" is called conditionally. React Hooks must be called in the exact same order in every component render. Did you accidentally call a React Hook after an early return?
() =>
voronoi<Datum>({
data,
Expand All @@ -49,9 +51,9 @@
[innerWidth, innerHeight],
);

const svgRef = useRef<SVGSVGElement>(null);

Check failure on line 54 in packages/visx-demo/src/sandboxes/visx-delaunay-voronoi/Example.tsx

View workflow job for this annotation

GitHub Actions / build 

React Hook "useRef" is called conditionally. React Hooks must be called in the exact same order in every component render. Did you accidentally call a React Hook after an early return?
const [hoveredId, setHoveredId] = useState<string | null>(null);

Check failure on line 55 in packages/visx-demo/src/sandboxes/visx-delaunay-voronoi/Example.tsx

View workflow job for this annotation

GitHub Actions / build 

React Hook "useState" is called conditionally. React Hooks must be called in the exact same order in every component render. Did you accidentally call a React Hook after an early return?
const [neighborIds, setNeighborIds] = useState<Set<string>>(new Set());

Check failure on line 56 in packages/visx-demo/src/sandboxes/visx-delaunay-voronoi/Example.tsx

View workflow job for this annotation

GitHub Actions / build 

React Hook "useState" is called conditionally. React Hooks must be called in the exact same order in every component render. Did you accidentally call a React Hook after an early return?

return width < 10 ? null : (
<svg width={width} height={height} ref={svgRef}>
Expand All @@ -71,7 +73,7 @@

const closest = voronoiDiagram.delaunay.find(point.x, point.y);
// find neighboring polygons to hightlight
if (closest && data[closest].id !== hoveredId) {
if (data[closest].id !== hoveredId) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

is find guaranteed to return something? if closest could be undefined, this would then throw an error when evaluating data[closest].id

Copy link
Author

Choose a reason for hiding this comment

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

In the voronoiDiagram.delaunay.find(point.x, point.y); line, the find method of the Delaunay class from d3-delaunay is being used, which is defined as follows in @types/d3-delaunay, guaranteeing that a number type will always be returned according to the type definition:

/**
 * Returns the index of the input point that is closest to the specified point ⟨x, y⟩.
 * The search is started at the specified point i. If i is not specified, it defaults to zero.
 */
find(x: number, y: number, i?: number): number;

DefinitelyTyped Definition

Furthermore, the actual d3-delaunay implementation is as follows: if ((x = +x, x !== x) || (y = +y, y !== y)) return -1; checks if x and y are numbers, and returns -1 if they are not. However, since this Demo is adopting TypeScript, I believe there should be no issue as long as it adheres to TypeScript's type checking.

find(x, y, i = 0) {
  if ((x = +x, x !== x) || (y = +y, y !== y)) return -1;
  const i0 = i;
  let c;
  while ((c = this._step(i, x, y)) >= 0 && c !== i && c !== i0) i = c;
  return c;
}

d3-delaunay Implementation

Additionally, this delaunay.find is already used as follows in the demo of visx-delaunay-triangulation, and there isn’t a check for undefined there.

const closest = delaunayDiagram.find(point.x - margin.left, point.y - margin.top);
setHoveredId(data[closest].id);

visx-delaunay-triangulation Demo

const neighbors = Array.from(voronoiDiagram.neighbors(closest));
setNeighborIds(new Set(neighbors.map((d) => data[d].id)));
setHoveredId(data[closest].id);
Expand Down