visgl / visgl/react-map-gl

[Feat] Deduplicate common code

Open
#2,488 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

feature
Dominant language
TypeScript
Stars
8.5k
Forks
1.4k
Avg merge
5d 17h
Merged PRs (30d)
3

Description

Target Use Case

I'm working on a library for a project that show a Mapbox or MapLibre map depending of a parameter. Since the update to mapbox-gl>=3.5.0 and react-map-gl 8.x, the duplication of functions and components pose us problems.

Here an example of what we're forced to do:

import { MapboxOverlay, type MapboxOverlayProps } from '@deck.gl/mapbox';
import {
    Map as MapGL,
    type MapProps as MapGlProps,
    type MapRef as MapGlRef,
    NavigationControl as NavigationControlGl,
    useControl as useControlGl,
} from 'react-map-gl/mapbox';
import {
    Map as MapLibre,
    type MapProps as MapLibreProps,
    type MapRef as MapLibreRef,
    NavigationControl as NavigationControlLibre,
    useControl as useControlLibre,
} from 'react-map-gl/maplibre';
// ...

type DeckGLOverlayProps = MapboxOverlayProps & { typeMap: TypeMap };
const DeckGLOverlay = forwardRef<MapboxOverlay, DeckGLOverlayProps>(({ typeMap, ...props }, ref) => {
    let overlay: MapboxOverlay;
    /* We can ignore ESlint rule react-hooks/rules-of-hooks here because useControl implementation is the same in the two cases
     * https://github.com/visgl/react-map-gl/blob/8.0-release/modules/react-mapbox/src/components/use-control.ts
     * https://github.com/visgl/react-map-gl/blob/8.0-release/modules/react-maplibre/src/components/use-control.ts
     */
    if (typeMap === TypeMap.GL) {
        overlay = useControlGl(() => new MapboxOverlay(props)); // eslint-disable-line react-hooks/rules-of-hooks
    } else {
        // @ts-expect-error: MapboxOverlay extends mapbox-gl.IControl while useControl ask for maplibre-gl.IControl ...
        overlay = useControlLibre(() => new MapboxOverlay(props)); // eslint-disable-line react-hooks/rules-of-hooks
    }
    overlay.setProps(props);
    return null;
});

// ...
    return (
        <Map>
            <DeckGLOverlay typeMap={props.mapLibrary === MAPBOX ? TypeMap.GL : TypeMap.Libre} /*...*/ />
            {props.mapLibrary === MAPBOX ? <NavigationControlGl /> : <NavigationControlLibre />}
            {/*...*/}
        </Map>
    );
}
Proposal

After looking quickly in the sources, it seems possible for some functions and components to be moved in an internal module "common".
Image

It is possible to de-duplicate some code like useControl and NavigationControl and move them in an internal module "common".

Then "react-mapbox" and "react-maplibre" modules would have as dependency this common module as a result.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by comparing modules/react-mapbox/src/components/use-control.ts and modules/react-maplibre/src/components/use-control.ts, then inspect the corresponding NavigationControl implementations. Determine which shared functions and components can move into an internal common module without changing either public module's behavior; done means both react-mapbox and react-maplibre consume the shared code without duplication.

Written by the indexing model from the issue text.

Assessment

Tech stack
react, typescript
Domain
frontend
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.