Fixing useRef() usage in createElementHook to prevent unnecessary Leaflet object creation

[abac]

• WAS: createElement() factory called at every render, therefore nullifying any benefits of using MutableRefObject
• NOW: createElement() factory called only if useRef() does not hold an already cached element object

[PaulLeCam]

Thanks but what's the purpose of this change please?

[abac]

Hello Paul, First of all, thank you for bringing and maintaining a very useful library. As it stands at the moment in react-leaflet, backing native leaflet objects are created every time (p)react calls render() method of the app component that uses react-leaflet components. Roughly, the call stack on render goes as follows: MyComponent.render() TileLayer.constructor() createTileLayerComponent::createElementHook() L.TileLayer.constructor() ... Most importantly, this is also the case when MyComponent is being re-rendered, e.g. due to a change in its state or properties. This slows down performance and creates potential for memory leaks. Fortunately, you are already using useRef() to hold native leaflet object in createElementHook(): in a rerender cycle, useRef returns existing native leaflet object as expected. Alas, the createElement callback is still being called without a check (since its value used in an initialValue parameter of useRef). I'm just adding that check and skipping potentially heavy createElement call.. Hope this makes sense and the new behaviour is by design that you've intended? Best, S

…

On Sat, 6 Aug 2022 at 14:28, Paul Le Cam ***@***.***> wrote: Thanks but what's the purpose of this change please? — Reply to this email directly, view it on GitHub <#1014 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABIJIDRC3F7V6NBH56NOZITVXZD65ANCNFSM55S7GNPQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[PaulLeCam]

Thanks for the details, ideally the createElement() call shouldn't be costly but I guess that's not always possible, so caching as you suggest sounds like a good idea.
Can you make sure the PR passes CI so II can merge it please?

[abac]

Cool, thank you for accepting the suggestion! As far as I could see, the CI errors are only formatting-related. Which tool are using to autoformat and is there a settings file that I could use for the project? Cheers Check failure on line 36 in packages/core/src/element.ts <https://github.com/PaulLeCam/react-leaflet/runs/7704496885?check_suite_focus=true> GitHub Actions/ Node 18.x on ubuntu-latest packages/core/src/element.ts#L36 Replace `LeafletElement<E>>;` with `⏎········LeafletElement<E>⏎······>` if (!elementRef.current) elementRef.current = createElement(props, context); Check failure on line 37 in packages/core/src/element.ts <https://github.com/PaulLeCam/react-leaflet/runs/7704496800?check_suite_focus=true> GitHub Actions/ Node 16.x on ubuntu-latest packages/core/src/element.ts#L37 Replace `·elementRef.current·=·createElement(props,·context);` with `⏎········elementRef.current·=·createElement(props,·context)`

…

On Sat, 6 Aug 2022 at 18:40, Paul Le Cam ***@***.***> wrote: Thanks for the details, ideally the createElement() call shouldn't be costly but I guess that's not always possible, so caching as you suggest sounds like a good idea. Can you make sure the PR passes CI so II can merge it please? — Reply to this email directly, view it on GitHub <#1014 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABIJIDVVTBJ55ZJDJJ6Z25LVX2BNDANCNFSM55S7GNPQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[PaulLeCam]

It's using ESLint, you should be able to reproduce the issue by running the "lint" script from the root package.json.

[abac]

Thanks for the pointer, Paul. Lint should be happy now.

[PaulLeCam]

Thanks!

[sherl0cks]

@PaulLeCam be advised that my team has a react-leaflet v3 app with lots of geojson layers that can be dragged around. This bug results in significant performance issues in under these conditions, which led us to write our own geojson component using the core API, but not the higher order hooks.

@abac Thank you very much for fixing this issue! I spent a considerable amount of time trying to find it but could not!