vector_math.dart icon indicating copy to clipboard operation
vector_math.dart copied to clipboard

Remove bundled dependencies

Open tvolkert opened this issue 2 years ago • 10 comments

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 avatar Aug 04 '22 01:08 tvolkert

@toji @kevmoo

tvolkert avatar Aug 04 '22 01:08 tvolkert

Internal bug: b/238779170

tvolkert avatar Aug 04 '22 01:08 tvolkert

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

  • those classes aren't referenced from flutter/flutter (or flutter/engine)
  • I don't see references to them from google3
  • I don't see references to them from the pub.dev packages that depend on vector_math (see https://gist.github.com/devoncarew/5c25a2fa7182c1bb6ea1b298ef5ece5b)

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 know if it's a drop-in replacement for the SimplexNoise classes, but I do see https://pub.dev/packages/fast_noise on pub.

devoncarew avatar Aug 12 '22 22:08 devoncarew

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:

  • those classes aren't referenced from flutter/flutter (or flutter/engine)
  • I don't see references to them from google3
  • I don't see references to them from the pub.dev packages that depend on vector_math (see https://gist.github.com/devoncarew/5c25a2fa7182c1bb6ea1b298ef5ece5b)

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: @.***>

kevmoo avatar Aug 12 '22 23:08 kevmoo

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:

  • user has a flutter app; they upgrade to the latest flutter version (which now has a dep on vector_math 3.0.0)
  • the user is also making use of a package, call it package:foo; package:foo has a dependency on package:vector_math
  • package:foo's dep on package:vector_math restricts it to the older major version (^2.0.0); additionally, package:foo is no longer maintained
  • when running flutter pub get after they upgrade to the latest flutter, pub can't solve and the user can no longer run their app

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:

  • come up with an indirection mechanism to help decouple flutter from directly pinning certain packages (cc @natebosch)
  • determine which of the 203 pub.dev packages using vector_math are also commonly used by flutter apps; contact those packages to widen their dep range on vector_math (>= 2.0.0 < 4.0.0)
  • 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

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 avatar Aug 15 '22 21:08 devoncarew

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).

devoncarew avatar Aug 16 '22 19:08 devoncarew

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

unicomp21 avatar Aug 19 '22 12:08 unicomp21

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.

devoncarew avatar Aug 19 '22 18:08 devoncarew

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

johnmccutchan avatar Sep 05 '22 16:09 johnmccutchan

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.

spydon avatar Feb 09 '23 23:02 spydon