React-leaflet: Extending components in V2 throws errors

[jbccollins]

• [x] All peer dependencies are installed: React, ReactDOM and Leaflet.
• [x] Using a supported version of React and ReactDOM (v16.3.0 minimum).
• [x] Using the supported version of Leaflet (v1.3.x) and its corresponding CSS file is loaded.
• [x] Using the latest stable v2 version of React-Leaflet - not v1.
• [x] The issue has not already been reported.
• [x] Make sure you have followed the quick start guide for Leaflet.
• [x] Make sure you have fully read the documentation and that you understand the limitations.

Expected behavior

Extending react-leaflet components should work

Actual behavior

Extending react-leaflet components throws errors

Steps to reproduce

V2 Code (not working):
https://codepen.io/anon/pen/WKqeWP

Old V1 Code (working):
https://jsfiddle.net/n95h2msu/

In V1 doing an extension like that of TileLayer would work. In V2 it does not. The documentation has no examples of extending a component that I could find. Please advise.

It would be super useful if https://react-leaflet.js.org/docs/en/custom-components.html contained a simple complete example of how to extend a component in V2.

[jbccollins]

Ok looks like this is kindof a duplicate of https://github.com/PaulLeCam/react-leaflet/issues/488 but that had no satisfying resolution

@PaulLeCam you say there

When creation custom components, you should extend one of the base classes, not one that is meant to be consumed directly

Meaning that to effectively 'extend' TileLayer I actually have to extend GridLayer and then copy/paste the createLeafletElement and updateLeafletElement functions from TileLayer into my custom component.

This feels wrong especially given that it means as leaflet and react-leaflet are updated I will need to manually keep my copy/pasted snippets up to date. Consider the use case where I don't want to change the createLeafletElement or updateLeafletElement functions. If all I want to do is add some code to the componentDidMount lifecycle function in TileLayer I would expect that I could do that just like I did in v1.

Unless I'm fundamentally missing something here this seemingly intended change feels like a major usability and maintainability regression from v1 ☹️

@PaulLeCam you said in #488

I don't think it's a good idea to have two GeoJSON exports, it's more confusing than anything.
If you want to implement your custom GeoJSON, you can start by copying over the current implementation and change what you need.

How would you feel about a changing the name of the extendable export so that it's more clear what's going on? Something like this:

import { TileLayer as LeafletTileLayer } from 'leaflet'

import { withLeaflet } from './context'
import GridLayer from './GridLayer'
import type { GridLayerProps } from './types'

type LeafletElement = LeafletTileLayer
type Props = { url: string } & GridLayerProps

class TileLayer extends GridLayer<LeafletElement, Props> {
  createLeafletElement(props: Props): LeafletElement {
    return new LeafletTileLayer(props.url, this.getOptions(props))
  }

  updateLeafletElement(fromProps: Props, toProps: Props) {
    super.updateLeafletElement(fromProps, toProps)
    if (toProps.url !== fromProps.url) {
      this.leafletElement.setUrl(toProps.url)
    }
  }
}

export { TileLayer as ExtendableTileLayer } // ADD THIS LINE!!
export default withLeaflet(TileLayer)

I could update the documentation to describe this behavior and add some example of extending both types of react-leaflet components.

[rubenspgcavalcante]

Got the same problem while trying to make react-leaflet-ant-path support the v2.
Trying to extend the Path Component results in this error:
Cannot read property 'layerContainer' of undefined

While trying to extend the Layer, it throws this error:
TypeError: Super expression must either be null or a function

[Update]
Figured out that in here:
https://github.com/PaulLeCam/react-leaflet/blob/c1e7fdc10f38c70e7c2cb266086931c13b55315e/src/MapLayer.js#L21-L23

The this.props is assuming the value of this.leafletElement.
The getter is getting a wrong context

[PaulLeCam]

v2 exposes 2 types of components: the "public API" ones that are meant to be consumed directly, and "abstract" ones that contain shared logic by the components extending them.

It works differently from v1 because of the context architecture changes introduced in React v16.3. If you know of a better way to handle this, please open a PR, but I don't like the option to export both "extendable" and "directly usable" version of a component, it's only going to make things more confusing.

v2 is a major version due to breaking changes and this is the main one, please don't expect things to work like v1.

[jbccollins]

@PaulLeCam I respectfully disagree with your decision.

For anyone who finds this issue in the future I have published a fork that lets you extend all components: https://github.com/jbccollins/react-leaflet-extendable

[jbccollins]

@Jcummings81 That's not really what this issue is about... But if you post a fiddle of your current map implementation using vanilla JS I can take a look

[Jcummings81]

Good morning James,

Sorry about that, new to the customs of stack overflow as well ha ha.
here is a link to a simple html example of using a map with the Leaflet
maps extension? called World. <
https://jsfiddle.net/cn0rw873/1/#&togetherjs=OdLPzEIlAu > I'm trying to
figure out how to implement this in a react component that only has a main
public index html file that I really don't want to alter. I'd like the JSX
translation of this. I can use Leaflet alone, but adding in World on top
of it seems to complicate it. For instance I can create a map with the
leaflet library alone with L.map but when I try to use L.Wrld.map react
gets confused and thinks I'm trying to map over an array.

Thank you for your response.

On Mon, Oct 1, 2018 at 7:17 PM James Collins [e-mail]
wrote:

@Jcummings81 https://github.com/Jcummings81 That's not really what this
issue is about... But if you post a fiddle of your current map
implementation using vanilla JS I can take a look

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/PaulLeCam/react-leaflet/issues/506#issuecomment-426114713,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AnkE3qf1CsF7srwYtDlTBbU-QrEtwc6Kks5ugr6PgaJpZM4V_HYF
.

[jbccollins]

Ok yeah https://jsfiddle.net/q2v7t59h/1942/

You're gonna need to extend react-leaflet's Map component and override the createLeafletElement function. I'm not familiar with Wrld but it's possible that you might also need to override the updateLeafletElement function although I think that's unlikely.

[Jcummings81]

Alright great, I'll look into that. Thank you again for your help.

On Tue, Oct 2, 2018, 7:39 AM James Collins [e-mail] wrote:

Ok yeah https://jsfiddle.net/q2v7t59h/1942/

You're gonna need to extend react-leaflet's Map component and override
the createLeafletElement function. I'm not familiar with Wrld but it's
possible that you might also need to override the updateLeafletElement
function although I think that's unlikely.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/PaulLeCam/react-leaflet/issues/506#issuecomment-426277144,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AnkE3nF-96yTDDcZb3hlsNensH8Xc26bks5ug2yTgaJpZM4V_HYF
.

[rjdestigter]

I'm really not liking having to copy over code to be able to extend.

[nuragic]

TBH I also disliked the idea of having to copy/paste the entire component code in order to modify some specific behavior, but then I realised that it's better to do so. Creating more levels of inheritance on top of the public API, which already have some of them to mimic how leaflet is built, is way worst in the end (see https://link.medium.com/iIALeQx38Q).

Tip: use https://github.com/flowtype/flow-remove-types if your source code is vanilla JS.

Also take care of your final production bundle size by not including those react-leaflet components that you're not using anymore.

[rjdestigter]

@nuragic I agree, but in that case I would still expose the functionality some of the classes implement (as functions maybe) for re-use to allow composition

[kjellski]

@PaulLeCam - after having a quick look at how @jbccollins forked and created this here - https://github.com/jbccollins/react-leaflet-extendable - which seems reasonable, how would I proceed without his fork?

I need to fix something on a component from a third party lib and I can't figure out a way to get the needed functionality of now unavailable createLeafletElement and updateLeafletElement.

[kjellski]

Hmmm, looking into this a bit more, I think @jbccollins made an even better point on this - you're hurting people helping this lib unintentionally - the plugin maintainers can't really help but maintain another lib as well as their own plugins functionality.

[stevus]

Should be composing rather than trying to extend of copy/paste react-leaflet source.

Most of the plugins out there are copying the entire source of a component, so every change to react-leaflet is potentially a breaking change'

const CustomMarker = (props) => {

    const openPopup = (marker) => {
        if (marker) {
            marker.leafletElement.openPopup()
        }
    }

    return (
        <Marker ref={el => openPopup(el)} {...props} />
    )
}