-
Notifications
You must be signed in to change notification settings - Fork 884
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.
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
Make Pane component compatible with StrictMode #810
Conversation
Thanks, can you please add tests for nested panes, children components other than 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. |
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
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 |
I've added tests for nested panes as well as unmounting and remounting. However, I didn't add tests for other children than |
This is already documented.
Also already documented. |
Wow, can't believe I missed that "Child components" page! Sorry for the noise. |
I ran into this problem too. Can I help to move this PR forward somehow? |
@PaulLeCam Not sure if you saw my comment above--I believe this is what's blocking:
|
I don't think mocking Leaflet is that useful to really prevent issues so I'm not doing in anymore in v3. |
@PaulLeCam I'm not sure what you're asking me to do. Any map layers that use SVG will currently fail because The only ways I can think of to add tests of SVG children are:
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. |
Headless browser tests would be good yes but I don't think that should be needed for this PR. 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. |
# Conflicts: # packages/react-leaflet/src/Pane.tsx
... 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 I have reverted these changes in the Pane component, but left them in the MapContainer component, in my merge commit. |
Thanks! |
Fixes #791.
This PR makes use of
useEffect
instead ofuseMemo
to handle the side effect of creating a Leaflet map pane in the<Pane>
component. This means that rendering that component does not fail inReact.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:
.tsx.snap
file.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.