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

Bump @mapbox/point-geometry from 0.1.0 to 1.0.0 #4393

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

dependabot[bot]
Copy link
Contributor

@dependabot dependabot bot commented on behalf of github Jul 11, 2024

Bumps @mapbox/point-geometry from 0.1.0 to 1.0.0.

Release notes

Sourced from @​mapbox/point-geometry's releases.

v1.0.0

  • ⚠️ Publish as an ES Module. Drop support for CommonJS.
  • ⚠️ Use modern ES syntax. You can still transpile on your end for IE11 support.
  • Add first-class TypeScript types.
Commits
Maintainer changes

This version was pushed to npm by mapbox-npm-06, a new releaser for @​mapbox/point-geometry since your current version.


Dependabot compatibility score

You can trigger a rebase of this PR by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

  • @dependabot rebase will rebase this PR
  • @dependabot recreate will recreate this PR, overwriting any edits that have been made to it
  • @dependabot merge will merge this PR after your CI passes on it
  • @dependabot squash and merge will squash and merge this PR after your CI passes on it
  • @dependabot cancel merge will cancel a previously requested merge and block automerging
  • @dependabot reopen will reopen this PR if it is closed
  • @dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
  • @dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency
  • @dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

Note
Automatic rebases have been disabled on this pull request as it has been open for over 30 days.

Bumps [@mapbox/point-geometry](https://github.com/mapbox/point-geometry) from 0.1.0 to 1.0.0.
- [Release notes](https://github.com/mapbox/point-geometry/releases)
- [Commits](mapbox/point-geometry@v0.1.0...v1.0.0)

---
updated-dependencies:
- dependency-name: "@mapbox/point-geometry"
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
@dependabot dependabot bot added dependencies Pull requests that update a dependency file javascript Pull requests that update Javascript code labels Jul 11, 2024
@github-actions github-actions bot enabled auto-merge (squash) July 11, 2024 08:45
@mourner
Copy link
Contributor

mourner commented Jul 12, 2024

Sorry for the trouble with the types!

Also heads up that we're holding off on merging a similar change in GL JS because benchmarks show a significant perf regression on Pixel 4 (an older phone, Chrome / Android — more recent hardware shows no difference). Not sure what the culprit is in those updates, might be the change from function Point(x, y) to class Point { constructor(x, y) since it's on a hot path but not sure yet.

@HarelM
Copy link
Collaborator

HarelM commented Jul 12, 2024

OK, thanks for the update!
I guess that if the point change is the root cause you'll publish a new version, which will be automatically merged here if it won't create any breaking changes.
I'll wait with this a bit just in case.

@HarelM
Copy link
Collaborator

HarelM commented Jul 12, 2024

Interestingly enough, this change caused the build size to increase dramatically too.
I'll see if I can spot why...

@mourner
Copy link
Contributor

mourner commented Jul 12, 2024

@HarelM I'm not sure what the root cause is yet — maybe you could run some benchmarks (general map rendering performance before/after the upgrades) on your side? Might help track it down.

@codecov-commenter
Copy link

codecov-commenter commented Jul 12, 2024

Codecov Report

Attention: Patch coverage is 86.66667% with 4 lines in your changes missing coverage. Please review.

Project coverage is 87.88%. Comparing base (3c5edcf) to head (2905ee0).
Report is 55 commits behind head on main.

✅ All tests successful. No failed tests found.

Files Patch % Lines
src/symbol/quads.ts 0.00% 2 Missing ⚠️
src/data/bucket/fill_extrusion_bucket.ts 50.00% 1 Missing ⚠️
src/source/vector_tile_worker_source.ts 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4393      +/-   ##
==========================================
+ Coverage   87.65%   87.88%   +0.22%     
==========================================
  Files         246      246              
  Lines       33375    33381       +6     
  Branches     2185     2181       -4     
==========================================
+ Hits        29255    29336      +81     
+ Misses       3123     3061      -62     
+ Partials      997      984      -13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@HarelM
Copy link
Collaborator

HarelM commented Jul 12, 2024

I'll start with the build size, as it should be easier to understand what changed, I hope. If I won't see anything out of the ordinary I'll turn to running the bench marks...

@mourner
Copy link
Contributor

mourner commented Jul 12, 2024

Interestingly enough, this change caused the build size to increase dramatically too.
I'll see if I can spot why...

This is likely because of vt-pbf, which was not updated and still has older pbf / vector-tile deps. In GL JS, we'll vendor the code instead of updating the library for now (eventually vt writing will likely move to vector-tile as a part of v3).

@HarelM
Copy link
Collaborator

HarelM commented Jul 12, 2024

Yes, it seems that pbf is indeed duplicated twice in the output file due to vt-pbf old dependency.
This old dependency also exitst with mvt-fixtures, but this package is used only in the tests, so I believe it is not to blame, but would be nice to align it as well I believe.
Is there anything I can help with in order to push the pbf version upgrade?

@HarelM
Copy link
Collaborator

HarelM commented Jul 12, 2024

I can't see a major difference in the benchmarks I run locally on my machine (which isn't saying much I guess).
I also tried benchmarking the change in point class here:
image

And it might be a bit slower, but not by a lot, it also changes a bit from run to run.
https://jsbench.me/

@HarelM
Copy link
Collaborator

HarelM commented Jul 12, 2024

In any case, the build size is currently what's holding this PR I believe.
Let me know if there's anything I can help out with to resolve this, if a PR to vt-pbf will help push that version bump forward I'll be happy to lay a hand.

Copy link
Contributor Author

dependabot bot commented on behalf of github Jul 17, 2024

A newer version of @​mapbox/point-geometry exists, but since this PR has been edited by someone other than Dependabot I haven't updated it. You'll get a PR for the updated version as normal once this PR is merged.

@HarelM
Copy link
Collaborator

HarelM commented Jul 29, 2024

@mourner any updates regarding vt-pbf? Anything I can do to push the update of pbf there?

@mourner
Copy link
Contributor

mourner commented Jul 29, 2024

I didn't manage to get to the bottom of the regression yet and put the upgrade on hold for now unfortunately (need to work on more urgent issues at the moment). Let me know if you find any clues.

@HarelM
Copy link
Collaborator

HarelM commented Jul 29, 2024

My question was more about vt-pbf and if I can help somehow to push forward the usage of an updated pbf package. I'm not too concerned about the regression in performance as developers who might need to support old devices might want to keep an old version of the library as well...

@mourner
Copy link
Contributor

mourner commented Jul 29, 2024

@HarelM that one's tricky — I wanted to update it but couldn't properly set up all the dev dependencies, e.g. @mapbox/vtvalidate seems broken with newer Node versions & the maintainer is currently on a parental leave.

@HarelM
Copy link
Collaborator

HarelM commented Jul 29, 2024

ahh, that has cpp stuff, so it's probably a pain :-/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants