Skip to content
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

fix: datasource context provider will now cleanup when being unmounted #126

Merged
merged 7 commits into from
Sep 15, 2022

Conversation

rvdkooy
Copy link

@rvdkooy rvdkooy commented Aug 12, 2022

<AzureMapDataSourceProvider /> does not work well with React.StrictMode

Strict mode will render, unmount and rerender again to find common issues see: https://reactjs.org/docs/strict-mode.html
When Strict mode is enabled, it will throw the following error: Uncaught Error: 'datasource-id' is already added to the map (map will not render anymore)

This PR fixes this issue by doing a cleanup when <AzureMapDataSourceProvider /> is being unmounted.

note: This PR does not fix any other cleanup issues in other components, so there could be more!

@ghost
Copy link

ghost commented Aug 12, 2022

CLA assistant check
All CLA requirements met.

@yulinscottkang
Copy link
Contributor

Thank you @rvdkooy! I was testing this PR with react-azure-maps-playground without strict mode and I got errors like the following while navigating between different examples. You're right, there are other cleanup issues and they might need to be fixed altogether at once.

Error: Source "BubbleLayer DataSourceProvider" cannot be removed while layer "BubbleLayer LayerProvider" is using it.
Uncaught Error: One or more layers have a dependency on the source 'BubbleLayer DataSourceProvider'

@rvdkooy
Copy link
Author

rvdkooy commented Aug 22, 2022

hi @yulinscottkang
I've been digging around on this issue and the problem is the order of unmounting / cleanup

<AzureMapDataSourceProvider> --> this unmounts first
  <AzureMapLayerProvider /> --> but this needs to be cleaned up first
</AzureMapDataSourceProvider>

In my latest commit, i've added some logic where the AzureMapDataSourceProvider will cleanup the layers from the map that are using the same datasource
This is still WIP, but I would like to have some feedback on this first!

Also had to do some updates on the playground see:
Azure/react-azure-maps-playground#77

@yulinscottkang
Copy link
Contributor

Thanks @rvdkooy, LGTM!

@rvdkooy
Copy link
Author

rvdkooy commented Aug 28, 2022

@yulinscottkang my latest commits should be fine and ready for review
Gr

getLayersDependingOnDatasource(dref).forEach((l) => {
mref.layers.remove(l.getId() ? l.getId() : l)
})
mref.sources.remove(dref)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rvdkooy, this is great!

Do you mind apply the same fix to AzureMapVectorTileSourceProvider.tsx as well? It can be tested with react-azure-maps-playground under http://localhost:3000/custom-traffic

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, wasn't able to have a look at it for a while.
I also fixed it in AzureMapVectorTileSourceProvider

Gr,
R

Copy link
Contributor

@yulinscottkang yulinscottkang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and LGTM! Thank you @rvdkooy.

@yulinscottkang yulinscottkang merged commit aa8c06d into Azure:master Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants