Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Updates the react-spatial package to require React 19+, modernizes tooling (ESLint 9 flat config, updated Jest config), and migrates repo workflows/docs from Yarn to pnpm.
Changes:
- Bump React peer deps to
^19and refresh related dependency/tooling versions. - Migrate scripts, docs, hooks, lint-staged, and CI from Yarn to pnpm (and add
packageManagerpin). - Adjust code/tests for new lint/runtime expectations (optional chaining, snapshot updates, test refactors).
Reviewed changes
Copilot reviewed 66 out of 71 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Adds TS config (noEmit/allowJs) to support updated tooling. |
| styleguide.config.js | Adds watch ignore patterns for faster styleguidist dev workflow. |
| src/utils/KML.js | Uses optional chaining to avoid null/undefined access. |
| src/styleguidist/StyleGuide.js | Refactors effects/refs for updated lint/runtime expectations. |
| src/styleguidist/ComponentsList.js | Removes unnecessary React import (new JSX runtime). |
| src/setupTests.js | Adds TextEncoder/TextDecoder globals for Jest environment. |
| src/components/Zoom/snapshots/Zoom.test.js.snap | Snapshot update for React/tooling changes. |
| src/components/Zoom/Zoom.test.js | Removes unnecessary async/await; lint suppressions added. |
| src/components/Zoom/Zoom.js | Removes eslint disable comment around prop spreading. |
| src/components/StopsFinder/snapshots/StopsFinder.test.js.snap | Snapshot update for React/tooling changes. |
| src/components/StopsFinder/StopsFinderOption.js | Refactors styled helper to satisfy linting. |
| src/components/StopsFinder/StopsFinder.js | Uses optional chaining for param serialization. |
| src/components/ScaleLine/snapshots/ScaleLine.test.js.snap | Snapshot header update. |
| src/components/ScaleLine/ScaleLine.test.js | Reorders imports (jest-canvas-mock). |
| src/components/ScaleLine/ScaleLine.js | Removes eslint disable comment around prop spreading. |
| src/components/RouteSchedule/RouteSchedule.test.js | Reorders imports (jest-date-mock). |
| src/components/RouteSchedule/RouteSchedule.js | Removes eslint disable comments around prop spreading. |
| src/components/ResizeHandler/ResizeHandler.test.js | Removes legacy eslint disables; minor cleanup. |
| src/components/ResizeHandler/ResizeHandler.js | Removes eslint disable comments around findDOMNode (commented code). |
| src/components/README.md | Updates component generator command to pnpm. |
| src/components/Popup/snapshots/Popup.test.js.snap | Snapshot header update. |
| src/components/Popup/Popup.test.js | Adds lint disables; optional chaining in teardown; moves jest-canvas-mock import. |
| src/components/Popup/Popup.js | Optional chaining for pixel array checks; removes eslint disable comment. |
| src/components/Permalink/Permalink.test.js | Reorders imports (jest-canvas-mock). |
| src/components/Permalink/Permalink.js | Removes eslint disable comments around param reassignment. |
| src/components/Overlay/snapshots/Overlay.test.js.snap | Snapshot header update. |
| src/components/NorthArrow/snapshots/NorthArrow.test.js.snap | Snapshot header update; removes one snapshot entry. |
| src/components/NorthArrow/NorthArrow.test.js | Reorders imports; removes one test; removes eslint disable comments. |
| src/components/NorthArrow/NorthArrow.js | Removes eslint disable comment around prop spreading. |
| src/components/MousePosition/snapshots/MousePosition.test.js.snap | Snapshot header update. |
| src/components/MousePosition/MousePosition.test.js | Reorders imports (jest-canvas-mock); removes eslint disable header. |
| src/components/MousePosition/MousePosition.js | Simplifies projection guard with optional chaining; removes eslint disable. |
| src/components/LayerTree/snapshots/LayerTree.test.js.snap | Snapshot header update. |
| src/components/LayerTree/LayerTree.test.js | Reorders imports; removes eslint disable header. |
| src/components/LayerTree/LayerTree.js | Refactors static helpers; uses optional chaining; removes some eslint disables. |
| src/components/Geolocation/snapshots/Geolocation.test.js.snap | Snapshot header update. |
| src/components/Geolocation/Geolocation.test.js | Reorders imports; optional chaining in teardown. |
| src/components/Geolocation/Geolocation.js | Removes eslint disable comment around prop spreading. |
| src/components/FitExtent/snapshots/FitExtent.test.js.snap | Snapshot header update; renames snapshot key. |
| src/components/FitExtent/FitExtent.test.js | Wraps tests in describe block; formatting cleanup. |
| src/components/FitExtent/FitExtent.js | Removes eslint disable comment around prop spreading. |
| src/components/FeatureExportButton/snapshots/FeatureExportButton.test.js.snap | Snapshot header update. |
| src/components/FeatureExportButton/FeatureExportButton.test.js | Reorders imports (jest-canvas-mock). |
| src/components/FeatureExportButton/FeatureExportButton.js | Removes eslint disable comments; keeps behavior. |
| src/components/Copyright/snapshots/Copyright.test.js.snap | Snapshot header update. |
| src/components/Copyright/Copyright.test.js | Reorders imports (jest-canvas-mock). |
| src/components/Copyright/Copyright.js | Refactors ref callback; removes eslint disable comment. |
| src/components/CanvasSaveButton/snapshots/CanvasSaveButton.test.js.snap | Snapshot header update. |
| src/components/CanvasSaveButton/CanvasSaveButton.test.js | Reorders imports; refactors async interactions. |
| src/components/CanvasSaveButton/CanvasSaveButton.js | Optional chaining for extraData; refactors array filtering; removes eslint disables. |
| src/components/BasicMap/BasicMap.test.js | Reorders imports (jest-canvas-mock). |
| src/components/BaseLayerSwitcher/BaseLayerSwitcher.test.js | Removes unnecessary async/await in fireEvent usage. |
| src/components/BaseLayerSwitcher/BaseLayerSwitcher.js | Removes eslint disable header/comments; keeps behavior. |
| scripts/read-pkg-json.js | Updates helper output from yarn commands to pnpm; formatting cleanup. |
| package.json | React 19 peer deps; pnpm migration; dependency/tooling bumps; adds packageManager. |
| jest.config.js | Updates transforms; removes unused moduleNameMapper entry. |
| eslint.config.mjs | Adds ESLint 9 flat config based on @geops/eslint-config-react. |
| doc/README.md | Updates dev command examples to pnpm. |
| README.md | Updates install command example to pnpm. |
| DEVELOP.md | Updates dev/test/coverage command examples to pnpm. |
| .nvmrc | Bumps Node version to 24. |
| .lintstagedrc.js | Runs related tests via pnpm instead of yarn. |
| .husky/post-rebase | Uses pnpm install after rebase. |
| .husky/post-merge | Uses pnpm install after merge. |
| .husky/post-checkout | Uses pnpm install after checkout. |
| .husky/commit-msg | Uses pnpm commitlint. |
| .gitignore | Ignores pnpm-workspace.yaml. |
| .github/workflows/main.yml | Switches CI commands from yarn to pnpm. |
| .eslintrc.js | Removes legacy ESLint config (replaced by flat config). |
Comments suppressed due to low confidence (1)
src/components/CanvasSaveButton/CanvasSaveButton.test.js:135
- This test schedules assertions inside
window.setTimeoutbut does not wait for that callback to run. Jest can finish the test before the expectations execute, leading to false positives or flakiness. Use fake timers withjest.runAllTimers()/advanceTimersByTime, or return/await a Promise (orwaitFor) that resolves after the timeout so the assertions are actually awaited.
window.setTimeout(() => {
expect(saveStart).toHaveBeenCalledTimes(1);
expect(saveEnd).toHaveBeenCalledTimes(1);
expect(link.href).toBe("http://localhost/fooblob");
expect(link.download).toBe(".jpg");
expect(link.click).toHaveBeenCalledTimes(1);
spy3.mockRestore();
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| </Paper> | ||
| <div className={classes.scrollable} ref={(nodee) => setNode(nodee)}> | ||
| <div | ||
| className={classes.scrollable} | ||
| ref={(nodee) => { | ||
| return setNode(nodee); |
There was a problem hiding this comment.
Ref callback functions should not return a value. Returning setNode(nodee) is ignored by React but can be misleading and may violate lint rules; just call setNode(nodee) in the callback (or pass setNode directly).
| ref={(nod) => { | ||
| return setNode(nod); | ||
| }} |
There was a problem hiding this comment.
Ref callback functions should not return a value. Returning setNode(nod) is ignored by React and is misleading; call setNode(nod) without returning anything (or use ref={setNode} if appropriate).
| ref={(nod) => { | |
| return setNode(nod); | |
| }} | |
| ref={setNode} |
| - name: Setup node | ||
| uses: actions/setup-node@v3 | ||
| with: | ||
| node-version-file: '.nvmrc' | ||
|
|
||
| - name: Run linting and tests | ||
| run: | | ||
| yarn install --frozen-lockfile | ||
| yarn lint | ||
| yarn test | ||
| pnpm install --frozen-lockfile | ||
| pnpm lint | ||
| pnpm test |
There was a problem hiding this comment.
CI now runs pnpm ... but the workflow does not install/activate pnpm (e.g., via corepack enable / corepack prepare pnpm@... --activate or pnpm/action-setup). On GitHub runners pnpm is not guaranteed to be available, so this job may fail.
| } | ||
| // eslint-disable-next-line consistent-return | ||
|
|
||
| return setSwitcherOpen(true) && setIsClosed(false); |
There was a problem hiding this comment.
setSwitcherOpen(true) && setIsClosed(false) will never call setIsClosed(false) because setSwitcherOpen returns undefined (falsy), so the && short-circuits. Call the setters in separate statements (and avoid returning their result) so both state updates happen reliably.
| return setSwitcherOpen(true) && setIsClosed(false); | |
| setSwitcherOpen(true); | |
| setIsClosed(false); |
danji90
left a comment
There was a problem hiding this comment.
Mostly good. Just some cleaning and eslint warnings to fix, and the LayerTree example is in need of a fix. Otherwise LGTM
| @@ -1,3 +1,3 @@ | |||
| import { fireEvent, render } from "@testing-library/react"; | |||
| import Layer from "ol/layer/Layer"; | |||
| import React from "react"; | |||
| ref={(nod) => { | ||
| return setNode(nod); | ||
| }} |
| // ol.layer.group | ||
| layer?.getLayers?.().getArray() || | ||
| []; | ||
| static getChildren = (layer) => { |
There was a problem hiding this comment.
Broken already before this update, but when you switch to Base Dark radio layer, then activate one of the checkbox child layers the base map is not set to visible and the map is white. Can you maybe fix this? Maybe we could even do a new example entirely, the current one is rather useless
How to
BREAKING CHANGES: react-spatial now supports only react >= 19 , use version 2.x.x for react 18 support
Others