Skip to content

Remove dependency on gazetteer - #1898

Merged
HarelM merged 1 commit into
maplibre:mainfrom
rotu:remove-gazetteer
Dec 11, 2022
Merged

Remove dependency on gazetteer#1898
HarelM merged 1 commit into
maplibre:mainfrom
rotu:remove-gazetteer

Conversation

@rotu

@rotu rotu commented Dec 1, 2022

Copy link
Copy Markdown
Contributor

Removes dependency on gazetteer, which was only being used for a single GeoJSON file for benchmarks but pulled in a number of deprecated packages.

Fix #1897

Launch Checklist

  • 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.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Manually test the debug page.
  • Add an entry to CHANGELOG.md under the ## main section.

@rotu
rotu force-pushed the remove-gazetteer branch from e6254e9 to 02e9dd9 Compare December 1, 2022 22:10
@rotu
rotu marked this pull request as ready for review December 1, 2022 22:14
@HarelM

HarelM commented Dec 1, 2022

Copy link
Copy Markdown
Collaborator

Thanks for taking the time to open this PR!
I'm not sure removing a benchmark is the right approach. These benchmarks help up make sure we don't introduce performance issues.
I would consider adding the file from the other repo here instead.

@github-actions

github-actions Bot commented Dec 1, 2022

Copy link
Copy Markdown
Contributor

Bundle size report:

Size Change: 0 B
Total Size Before: 206 kB
Total Size After: 206 kB

Output file Before After Change
maplibre-gl.js 197 kB 197 kB 0 B
maplibre-gl.css 9.1 kB 9.1 kB 0 B
ℹ️ View Details No major changes

@rotu

rotu commented Dec 2, 2022

Copy link
Copy Markdown
Contributor Author

@HarelM Good points. I'm skeptical that this file has enough going on in it to provide meaningful performance metrics. Is there a meatier GeoJSON file to use instead?

@HarelM

HarelM commented Dec 2, 2022

Copy link
Copy Markdown
Collaborator

@wipfli @xabbu42 do you guys have input on this?

@xabbu42

xabbu42 commented Dec 2, 2022

Copy link
Copy Markdown
Collaborator

The removed benchmark is actually very valuable and has itself nothing todo with geojson. The geojson is only used as a list of locations to benchmark. I agree that we should just include a copy of the file in this repository, which also would allow us the add/remove locations from the benchmark if we want to.

Comment thread test/bench/versions/benchmarks.ts
@rotu
rotu force-pushed the remove-gazetteer branch from e84ed17 to 5a2b22d Compare December 5, 2022 05:04
@rotu

rotu commented Dec 5, 2022

Copy link
Copy Markdown
Contributor Author

@xabbu42 I totally misunderstood you - thanks for your patience! Yes, it does seem this benchmark does something, though I still can't say I truly grasp how the benchmarks work!

@xabbu42

xabbu42 commented Dec 5, 2022

Copy link
Copy Markdown
Collaborator

@rotu no worries, thanks for taking the time to improve maplibre!

Comment thread test/bench/styles/benchmarks.ts
@rotu rotu mentioned this pull request Dec 11, 2022
9 tasks
@HarelM
HarelM merged commit b1a5851 into maplibre:main Dec 11, 2022
@rotu
rotu deleted the remove-gazetteer branch December 19, 2022 20:48
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.

Unnecessary dependency on gazetteer

4 participants