ERA-10349: Support EFB schemas in ER > Location Field [2] #1263
Conversation
…in the new useLocationMarkersLayer hook when closing and opening events
| onBlur(blurEvent); | ||
| // Add a mouse down event to close the menu if the user clicks outside only if the user is not picking a location | ||
| // from the map. | ||
| if (!isPickingLocation) { |
There was a problem hiding this comment.
This fixes the issue when chosing a location and clicking something that is not the map. We just remove the listener while user picks a location so its logic doesn't un-focuses the location picker getting the app into a broken state.
| return null; | ||
| }, [eventLocation?.latitude, eventLocation?.longitude, markers]); | ||
| // GeoJSON feature collection with line strings connecting each marker to the event location if it is defined. | ||
| const markerConnectingLinesFeatureCollection = useMemo(() => featureCollection( |
There was a problem hiding this comment.
This fixes the source data inconsistency issue. I was returning null for the source data, which isn't handled properly by mapbox. Returning an empty feature collection instead works fine 😄
| updateLocationMarkers | ||
| } = useLocationMarkersLayer(eventLocation, onLocationMarkerClick); | ||
| setLocationMarkers, | ||
| } = useMapLocationMarkers(eventId, eventLocation, onLocationMarkerClick, hideMapLocationMarkers); |
There was a problem hiding this comment.
The useMapLocationMarkers hook now receives the eventId and a flag to hideMapLocationMarkers. Previously, the source and layer ids where constants, meaning that several instances of the hook couldn't be mounted at the same time. Now the eventId makes each source and layer id even if the hook mounts multiple times. The hideMapLocationMarkers is used for the scenario where one event is added over another (clicking the Event button in the footer of a Report Detail View), so the event that is now behind doesn't show its markers.
| const MARKER_CONNECTING_LINES_LAYER_ID = `${LAYER_IDS.EVENT_LOCATION_MARKERS}-lines`; | ||
| const MARKER_CONNECTING_OUTLINES_LAYER_ID = `${LAYER_IDS.EVENT_LOCATION_MARKERS}-outlines`; | ||
|
|
||
| const useMapLocationMarkers = (eventId, eventLocation, onMarkerClick = null, hideLayers = false) => { |
There was a problem hiding this comment.
The hook was renamed and improved to support multiple mounts and hide layers. Also it doesn't use useMapSource nor useMapLayer anymore since those hooks aren't stable and don't clean the map memory correctly.
There was a problem hiding this comment.
It would be a great idea to have a ticket/bug to stabilize useMapSource and useMapLayer hooks since are widely used in the app, and it will reduce significantly the logic in this useMapLocationMarkers hook
There was a problem hiding this comment.
Yeah, that is a fantastic idea. However, that ticket wouldn't be simple. Let me explain a little:
The flow to add icons, geoJSON, or anything else to Mapbox should be as follows:
1- Wait for the map to load.
2- Add a source to the map with its data. map.addSource(...);
2.1- Update the source data whenever it is necessary map.getSource(...).setData(...);
3- Add a layer to the map linked to the source and the desired layout and paint properties. map.addLayer(...);
3.1 Update the layer properties whenever it is necessary map.setLayoutProperty(...);
4- Remove the layer when it is no longer necessary map.removeLayer(...);
5- Remove the source when it is no longer necessary map.removeSource(...);
That is not only the happy path, but the only function path. If you try to remove a source before removing all the layers attached to it, it won't be removed. If you try to update a layer before it is added, it will throw an exception. We need to follow that specific order.
Now, that is impossible with our hooks. We have a useMapSources() and a useMapLayers() hooks that try to follow that flow with a set of useEffects. These hooks enforce you to add the sources when mounting and remove them when unmounting.
useMapSources(...); // It adds the sources on mount and tries to remove them on unmount but that will fail!!!
useMapLayers(...); // It adds the layers on mount and removes them on unmount.
The removal of the sources will fail because of the order of the hooks. We need to first remove the layers, but the hook goes after so it will never clean the layers before the sources. If we put the useMapLayers first, then when we try to add the layers to the source it will fail because the sources haven't been added yet. The issue is to try to have the sources and layers in isolated hooks. They can't be isolated! Their flows are mixed one with the other so we need to handle them in harmony.
And that is not the only issue. Also we have the amount of unnecessary useEffects we are attaching to the lifecycle of the component that uses these hooks. Just take a look at the useMapLayers/index.js file. It has like 6 useEffects that are processing on each render and in most cases we don't really need what some of them are doing, for example setPaintProperty or setFilter. We may use some of them in a few scenarios, but never all of them. Take as example this PR, I only need to add two sources, then three layers, and every now and then update the data of the sources and a layout property of the layer. Why would I want useEffects running to update filters or paint properties if I don't need them?
In my opinion, the solution is to get rid of those hooks. I understand they try to encapsulate a common pattern for creating sources and layers, but the implementation of them is not really common across the app. Every single source and layer behaves differently, refreshes their data differently and uses different layer properties to draw in the map. Once you understand how Mapbox works, sources and layers are not scary at all and I think we should feel good using the API directly without trying to mask it with these hooks that try to give a "one solution to all problems" that end up causing memory leaks, console clutter with warnings and errors, and unexpected behavior.
Sorry for the long response, this may have been easier in a call. We can talk again about this anytime, and I would be happy to work on a ticket. I'm pretty sure that would result in map performance improvement (saving tons of useEffects running on each render and probably updating source data when its no necessary), but it's going to take a while to rework all the places where we already use these hooks.
| const MARKER_CONNECTING_LINES_LAYER_ID = `${LAYER_IDS.EVENT_LOCATION_MARKERS}-lines`; | ||
| const MARKER_CONNECTING_OUTLINES_LAYER_ID = `${LAYER_IDS.EVENT_LOCATION_MARKERS}-outlines`; | ||
|
|
||
| const useMapLocationMarkers = (eventId, eventLocation, onMarkerClick = null, hideLayers = false) => { |
There was a problem hiding this comment.
It would be a great idea to have a ticket/bug to stabilize useMapSource and useMapLayer hooks since are widely used in the app, and it will reduce significantly the logic in this useMapLocationMarkers hook
| @@ -70,7 +70,5 @@ describe('TrackLegend - TimeOfDaySettings - TimeZoneSelect', () => { | |||
| userEvent.click(screen.getAllByRole('option')[100]); | |||
|
|
|||
| expect(setTimeOfDayTimeZone).toHaveBeenCalledTimes(1); | |||
| // This test may break if someday the IANA standard updates. | |||
| expect(setTimeOfDayTimeZone).toHaveBeenCalledWith('America/Marigot'); | |||
There was a problem hiding this comment.
What happened here, why to delete this assert?
There was a problem hiding this comment.
This is the line that has been bothering us for a couple weeks with tests breaking in different timezone servers. Joshua sent a fix in another PR removing the entire test, but that is not necessary, removing this line is enough and we still keep some coverage 😄
What does this PR do?
Fixes 2 bugs found during the testing phase of this ticket:
Relevant link(s)