Skip to content

fix: max depth exceeded error when dynamically changing map settings #2535

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

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

fix: max depth exceeded error when dynamically changing map settings #2535

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3220338
fix recursion
hwandrew May 20, 2025
5a62b19
Merge branch 'master' into ah/fix-recursion
hwandrew Oct 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions modules/react-mapbox/src/mapbox/mapbox.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,9 +216,6 @@
this.props = props;

const settingsChanged = this._updateSettings(props, oldProps);
if (settingsChanged) {
this._createProxyTransform(this._map);
Copy link
Contributor

Choose a reason for hiding this comment

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

@Pessimistress do you know why we had this?

}
const sizeChanged = this._updateSize(props);
const viewStateChanged = this._updateViewState(props, true);
this._updateStyle(props, oldProps);
Expand Down Expand Up @@ -502,7 +499,7 @@
@param {object} currProps
@returns {bool} true if anything is changed
*/
_updateStyleComponents(nextProps: MapboxProps, currProps: MapboxProps): boolean {

Check warning on line 502 in modules/react-mapbox/src/mapbox/mapbox.ts

View workflow job for this annotation

GitHub Actions / test-node 

Method '_updateStyleComponents' has a complexity of 13. Maximum allowed is 11
const map = this._map;
let changed = false;
if (map.isStyleLoaded()) {
Expand Down
36 changes: 36 additions & 0 deletions modules/react-mapbox/test/components/map.spec.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,42 @@ test('Map#controlled#no-update', t => {
);
});

test('Map#uncontrolled#delayedSettingsUpdate', async t => {
const root = createRoot(document.createElement('div'));
const mapRef = {current: null};

function App() {
const [settings, setSettings] = React.useState({
maxPitch: 85
});

async function onLoad() {
await sleep(1);
Copy link

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

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

Using very short arbitrary sleeps (1ms) increases test flakiness; consider awaiting a specific condition (e.g., poll until mapRef.current.getMaxPitch() === 60) or using a higher-level helper instead of fixed tiny delays.

Copilot uses AI. Check for mistakes.

setSettings({maxPitch: 60});
}

return (
<Map
ref={mapRef}
mapLib={import('mapbox-gl-v3')}
Copy link

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

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

Calling import('mapbox-gl-v3') inside render will create a new Promise on every re-render when settings change. Memoize it (e.g., const mapLib = React.useMemo(() => import('mapbox-gl-v3'), []);) or move it outside the component to avoid repeated dynamic import calls.

Copilot uses AI. Check for mistakes.

mapboxAccessToken={MapboxAccessToken}
initialViewState={{longitude: -100, latitude: 40, zoom: 4}}
{...settings}
onLoad={() => {
onLoad();
}}
Comment on lines +171 to +173
Copy link

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

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

Redundant arrow wrapper calling onLoad; you can pass the onLoad function directly (onLoad={onLoad}) to simplify and avoid an unnecessary redefinition each render.

Suggested change
onLoad={() => {
onLoad();
}}
onLoad={onLoad}

Copilot uses AI. Check for mistakes.

/>
);
}

root.render(<App />);

await waitForMapLoad(mapRef);
await sleep(1);
Copy link

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

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

Using very short arbitrary sleeps (1ms) increases test flakiness; consider awaiting a specific condition (e.g., poll until mapRef.current.getMaxPitch() === 60) or using a higher-level helper instead of fixed tiny delays.

Copilot uses AI. Check for mistakes.


t.is(mapRef.current.getMaxPitch(), 60, 'maxPitch is updated');
});

test('Map#controlled#mirror-back', t => {
const root = createRoot(document.createElement('div'));
const mapRef = {current: null};
Expand Down
Loading