google / vector_math.dart

A vector math package for 2D and 3D applications
https://pub.dev/packages/vector_math
Other
311 stars 78 forks source link

Remove bundled dependencies #269

Open tvolkert opened 2 years ago

tvolkert commented 2 years ago

https://github.com/google/vector_math.dart/tree/master/lib/src/vector_math/third_party https://github.com/google/vector_math.dart/tree/master/lib/src/vector_math_64/third_party

We should strip out these dependencies and import them individually from a separately package. This package may already exist.

Bundled dependencies make it difficult to properly import packages into other repositories (such as Google's) due to potential licensing issues. Bundled dependencies can also represent potential security concerns, for example if a vulnerable older version of a library is bundled.

tvolkert commented 2 years ago

@toji @kevmoo

tvolkert commented 2 years ago

Internal bug: b/238779170

devoncarew commented 2 years ago

It looks like those libraries just contribute SimplexNoise and vector_math_64 SimplexNoise to the API. From some brief investigation:

I think just removing them from the API is a feasible solution. So, something like:

We'd probably also want to update the readme to mention the removed API as people might look there if they hit issues after a pub upgrade. And I don't know if it's a drop-in replacement for the SimplexNoise classes, but I do see https://pub.dev/packages/fast_noise on pub.

kevmoo commented 2 years ago

SGTM, Devon.

On Fri, Aug 12, 2022 at 5:40 PM Devon Carew @.***> wrote:

It looks like those libraries just contribute SimplexNoise and vector_math_64 SimplexNoise to the API. From some brief investigation:

I think just removing them from the API is a feasible solution. So, something like:

  • deprecating the classes and publish a new minor version (~2.2.0)
  • removing the vendored libraries and publishing a new major version (3.0.0)

We'd probably also want to update the readme to mention the removed API as people might look there if they hit issues after a pub upgrade. And I don't if it's a drop-in replacement for the SimplexNoise classes, but I do see https://pub.dev/packages/fast_noise on pub.

— Reply to this email directly, view it on GitHub https://github.com/google/vector_math.dart/issues/269#issuecomment-1213570373, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAEFCRSR2ZW4AOU7CXFYK3VY3HGRANCNFSM55Q2EAFQ . You are receiving this because you were mentioned.Message ID: @.***>

devoncarew commented 2 years ago

After a discussion w/ @natebosch, I'm reminded that it's disproportionally expensive to ship a major version rev of a package that's been pinned by flutter. Currently, those packages are:

  characters: 1.2.1
  collection: 1.16.0
  material_color_utilities: 0.2.0
  meta: 1.8.0
  vector_math: 2.1.2

The expense comes from:

One shorter term solution to this problem for the user is to use a dependency override (dependency_overrides: vector_math: 3.0.0); but that solution likely won't occur to most users.


We'll need to think about how to move forward here a bit more, but a few short and long term options are:

We'll probably want to deprecate the API whatever path we choose, and, could gather more info about the actual impact of a major version rev. of this package.

devoncarew commented 2 years ago

Some slightly updated data:

All the packages using vector_math, whose deps resolve against the current stable dart and flutter sdks, and have been published in the last 12 months: https://gist.github.com/devoncarew/25cd4bbf2fe3342f3fcd2b4fa4d10de3 (111 packages)

All the packages using vector_math, whose deps resolve against the current stable dart and flutter sdks, including older (not recently published) packages: https://gist.github.com/devoncarew/bf014475a01c61f802e4a3c080613ed3 (197 packages)

Note that both of these new queries show two existing uses of the SimplexNoise classes (from package:flame and package:flamemodify).

unicomp21 commented 2 years ago

Did we bother to create a separate package for simplex noise?

devoncarew commented 2 years ago

We don't plan to, but if one is created, it would be good to update the deprecation issue (https://github.com/google/vector_math.dart/issues/270) with that info.

johnmccutchan commented 2 years ago

I strongly agree with the removal of simplex noise. Thanks @devoncarew

spydon commented 1 year ago

remove the SimplexNoise classes in the current major version; this wouldn't follow semver but in practice would likely be less of a breaking change

I strongly advice against this way forward (to not follow semver), even if we would be quick to push out a new version of Flame it would break all older Flame versions.