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

Remove bundled dependencies #269

Open
tvolkert opened this issue Aug 4, 2022 · 10 comments
Open

Remove bundled dependencies #269

tvolkert opened this issue Aug 4, 2022 · 10 comments

Comments

@tvolkert
Copy link
Collaborator

tvolkert commented Aug 4, 2022

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
Copy link
Collaborator Author

tvolkert commented Aug 4, 2022

@toji @kevmoo

@tvolkert
Copy link
Collaborator Author

tvolkert commented Aug 4, 2022

Internal bug: b/238779170

@devoncarew
Copy link
Collaborator

devoncarew commented Aug 12, 2022

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 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
Copy link
Collaborator

kevmoo commented Aug 12, 2022 via email

@devoncarew
Copy link
Collaborator

devoncarew commented Aug 15, 2022

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
Copy link
Collaborator

devoncarew commented Aug 16, 2022

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
Copy link

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

@devoncarew
Copy link
Collaborator

devoncarew commented Aug 19, 2022

We don't plan to, but if one is created, it would be good to update the deprecation issue (#270) with that info.

@johnmccutchan
Copy link
Collaborator

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

@spydon
Copy link
Collaborator

spydon commented Feb 9, 2023

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.

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

No branches or pull requests

6 participants