Make Pane component compatible with StrictMode

[zmbc]

Fixes #791.

This PR makes use of useEffect instead of useMemo to handle the side effect of creating a Leaflet map pane in the <Pane> component. This means that rendering that component does not fail in React.StrictMode and also will make it compatible in the future with Concurrent Mode.

Note that there are still several bugs/instances of weird behavior that this does not fix:

• Panes do not actually respond to updated props--name, style, parent pane are all fixed for the life of the component
• Panes without names do not actually contain their children. You can see this bug in the .tsx.snap file.
• Panes do not respond to context changes, nor do they pass updated leaflet context onto their children--they always pass the context at the time they were instantiated. Not sure how big of issues this causes.

All of the above could be fixed by more in-depth changes along the same lines, which I am happy to do if this is well received.

[PaulLeCam]

Thanks, can you please add tests for nested panes, children components other than <TileLayer> and unmounting/mounting again panes with the same name? These are some of the more complex cases the current behavior should handle.

About the immutable props, this is expected, it's safer to make them immutable for consistency. Regarding the parent context I don't think there are changes that should affect the behavior but in case it could be made mutable.
Also the name prop is required, it's not supposed to work without.

[zmbc]

About the immutable props, this is expected, it's safer to make them immutable for consistency.

This makes sense to me, and I can't think of a reason why I'd want mutable props, but this should definitely be documented as it is not the standard/expected behavior of React components. I'd be happy to write something, but I wasn't able to find any API documentation on <Pane> except for the example--maybe I could add a note as a comment in that code if there's nowhere else more appropriate?

Also the name prop is required, it's not supposed to work without.

Ah, I see. Unfortunately, any project not using TypeScript isn't aware of this, because there is no runtime check. There's a few different strategies I see on the web for reflecting TypeScript prop types as React.propTypes to add a runtime check--maybe this is of interest?

[zmbc]

I've added tests for nested panes as well as unmounting and remounting.

However, I didn't add tests for other children than <TileLayer> because they won't render in the test environment. It seems you have fixed this problem before--do you have a newer approach? There's nothing for me to model my tests on, as there are no existing tests for any other kinds of layers.

[PaulLeCam]

About the immutable props, this is expected, it's safer to make them immutable for consistency.

This makes sense to me, and I can't think of a reason why I'd want mutable props, but this should definitely be documented as it is not the standard/expected behavior of React components. I'd be happy to write something, but I wasn't able to find any API documentation on <Pane> except for the example--maybe I could add a note as a comment in that code if there's nowhere else more appropriate?

This is already documented.

Also the name prop is required, it's not supposed to work without.

Ah, I see. Unfortunately, any project not using TypeScript isn't aware of this, because there is no runtime check. There's a few different strategies I see on the web for reflecting TypeScript prop types as React.propTypes to add a runtime check--maybe this is of interest?

Also already documented.
Regarding proptypes, React and the community seem to move away from them so I don't want to add support and extra dependencies.

[zmbc]

Wow, can't believe I missed that "Child components" page! Sorry for the noise.

[anajavi]

I ran into this problem too.

Can I help to move this PR forward somehow?

[zmbc]

@PaulLeCam Not sure if you saw my comment above--I believe this is what's blocking:

However, I didn't add tests for other children than <TileLayer> because they won't render in the test environment. It seems you have fixed this problem before--do you have a newer approach? There's nothing for me to model my tests on, as there are no existing tests for any other kinds of layers.

[PaulLeCam]

I don't think mocking Leaflet is that useful to really prevent issues so I'm not doing in anymore in v3.
For panes the docs example has been useful to check the behavior, mostly related to mounting/unmounting.
If you can figure out a good way to get the exact behavior in unit tests that would be great, please add it to your PR!

[zmbc]

@PaulLeCam I'm not sure what you're asking me to do.

Any map layers that use SVG will currently fail because jsdom's SVG support is still a work in progress.

The only ways I can think of to add tests of SVG children are:

1. Mocking either Leaflet or, more minimally, createSVGRect. Of course, mocking won't actually render the correct SVG so some issues might not be caught and snapshots will not be accurate.
2. Using some kind of unstable/unreleased version of jsdom.
3. Running these tests in an actual headless browser instead of jsdom.

Would you like me to attempt to do one of these three?

Personally, if it were me, I'd just merge this PR without having all those tests since the jsdom+SVG issue is unrelated to panes and at least this PR adds some pane tests, where the existing code has none. But it's your call, obviously.

[PaulLeCam]

Headless browser tests would be good yes but I don't think that should be needed for this PR.
All I'm concerned about is that this change could break the behavior or other components. With the logic you're changing in effect every pane will first not render, and only setting its element in the effect callback to then re-render, which is a behavior I wanted to avoid.

Hopefully it will work as expected, I'm up for making a release with this but I'll revert it if it breaks anything else.
Can you resolve the conflict please so I can merge the PR?

[zmbc]

... I'm confused. Looking at the merge conflicts, I see that all useMemo instances were replaced by useState. (This doesn't fix the StrictMode issue, because useState initialization callbacks are also not safe places to perform side effects.)

I don't understand this. Using useMemo to store newContext makes less sense than using useState--it is a cached function result, not a mutable state. useMemo was not the right tool for the paneElement because it performed a side effect, which newContext does not.

I have reverted these changes in the Pane component, but left them in the MapContainer component, in my merge commit.

[PaulLeCam]

Thanks!