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

Break circular dep: index.ts ... web_worker.ts -> src/index.ts #2156

Merged
merged 1 commit into from
Feb 9, 2023

Conversation

zhangyiatmicrosoft
Copy link
Contributor

@zhangyiatmicrosoft zhangyiatmicrosoft commented Feb 9, 2023

Launch Checklist

Long circular depedency:
src/index.ts -> src/ui/map.ts -> src/style/style.ts -> src/util/global_worker_pool.ts -> src/util/worker_pool.ts -> src/util/web_worker.ts -> src/index.ts

It looks very reasonable to move workerURL to config object instead of piggyback on main maplibre.

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.

@HarelM
Copy link
Collaborator

HarelM commented Feb 9, 2023

There's a need to update the documentation I believe and also a note in the changelog that this is a breaking change.
I would recommend opening an issue in the docs repo after this is merged so that we don't forget to update the documentation there (about the workers, csp etc).
Overall, this look good. THANKS!

@HarelM
Copy link
Collaborator

HarelM commented Feb 9, 2023

A side note, this will also cause conflicts with #2130

@zhangyiatmicrosoft
Copy link
Contributor Author

zhangyiatmicrosoft commented Feb 9, 2023

Regarding doc changes, actually no need, I think.
Everyone can still use maplibre.workerUrl = "foo", it is just wraps and sends to config object. No one external or internal needs to and should care.
I have been trying to keep it exactly as before and reduce entries in Changelog to only absolutely necessary.

@zhangyiatmicrosoft
Copy link
Contributor Author

A side note, this will also cause conflicts with #2130

I will work with him to get this correctly merged. Thanks

@HarelM
Copy link
Collaborator

HarelM commented Feb 9, 2023

My mistake, I complete read the code wrong. Thanks for pointing this out! :-)

@HarelM HarelM merged commit b5cc0c1 into maplibre:main Feb 9, 2023
@rotu
Copy link
Contributor

rotu commented Feb 9, 2023

I like this change :-) Thank you!

@zhangyiatmicrosoft zhangyiatmicrosoft deleted the dep0209 branch February 10, 2023 01:07
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.

3 participants