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

fix(deps): Drop explicit dependency on vector #69

Merged

Conversation

bmillwood
Copy link
Contributor

I wanted compatibility with vector-0.13. To check whether any of the vector-0.13 changes were breaking, I reviewed where the existing code uses Data.Vector, and the answer is almost nowhere, and all existing use cases can be replaced with typeclass methods that don't require explicitly importing the module. As such, we can get away without an explicit direct dependency on vector, which should lighten the maintenance burden of getting the version bounds correct.

Strictly speaking it is possible to imagine a future vector upgrade breaking this package, so we do lose something by dropping the version bounds entirely. But since the functionality we use from the module is so basic, it's a little hard to imagine that happening in practice. I confirmed that the oldest version of aeson allowed (1.4.7.1) requires a version of vector (0.12.0.1) that has both IsList and Foldable, and the oldest version of base allowed (4.13) still has the polymorphic any.

In addition, it's possible that switching from monomorphic functions to typeclass methods could have performance implications, but I don't think this code is particularly perf-sensitive, and anyway inlining may yet be enough to save us.

Requirements

  • I have added test coverage for new or changed functionality (none)
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions (I'm hoping CI will do this for me)

Related issues

(I didn't open one, but will do if this needs more discussion / thought).

Describe the solution you've provided

The package now builds with vector-0.13.

Describe alternatives you've considered

Just widening the constraint to allow 0.13, which almost certainly would also be fine. (But would bring us back here when 0.14 comes out...)

Additional context

(none)

I wanted compatibility with vector-0.13. To check whether any of the vector-0.13
changes were breaking, I reviewed where the existing code uses Data.Vector, and
the answer is almost nowhere, and all existing use cases can be replaced with
typeclass methods that don't require explicitly importing the module. As such,
we can get away without an explicit direct dependency on vector, which should
lighten the maintenance burden of getting the version bounds correct.

Strictly speaking it is possible to imagine a future vector upgrade breaking
this package, so we do lose something by dropping the version bounds entirely.
But since the functionality we use from the module is so basic, it's a little
hard to imagine that happening in practice.

In addition, it's possible that switching from monomorphic functions to
typeclass methods could have performance implications, but I don't think this
code is particularly perf-sensitive, and anyway inlining may yet be enough to
save us.
@bmillwood bmillwood requested a review from a team as a code owner January 30, 2024 19:25
@bmillwood bmillwood changed the title Drop explicit dependency on vector (to allow building with vector-0.13) build: Drop explicit dependency on vector (to allow building with vector-0.13) Jan 30, 2024
@bmillwood
Copy link
Contributor Author

BTW, I plan to start using this package with GHC 9.4, which isn't currently covered by the CI matrix. Happy to include that in this PR or another one if you'd like -- looks like lts-21.25 on ghc-9.4.8 is the latest stack resolver. (Not sure what your policy is on keeping old GHC versions in there.)

(Also, weirdly, the quality-checks stuff does use GHC 9.4)

@keelerm84
Copy link
Member

Thank you for taking the time to make this contribution!

The lowest stack resolver we have a target for is LTS 16.31. That has a base of 4.13.0.0. GHC.IsList wasn't included until base-4.17.0.0. That would bring the minimum stack LTS resolver up to the 21.x branch.

That seems like something we would need to reserve for a major breaking change. We could expand the existing bounds for this current release, as you mentioned, and then use this as the basis for the next major version.

Not sure what your policy is on keeping old GHC versions in there.

We don't have a well-defined policy when it comes to supporting old GHC versions, or older stack resolvers, etc. I would be curious to hear, as a member of the Haskell community, what your preferred support policy would be for a library like ours?

This is where it was prior to base-4.17.0.0 (and can still be found
now), so this helps us to retain compatibility with older base versions.
@bmillwood
Copy link
Contributor Author

bmillwood commented Feb 1, 2024

The lowest stack resolver we have a target for is LTS 16.31. That has a base of 4.13.0.0. GHC.IsList wasn't included until base-4.17.0.0. That would bring the minimum stack LTS resolver up to the 21.x branch.

That seems like something we would need to reserve for a major breaking change. We could expand the existing bounds for this current release, as you mentioned, and then use this as the basis for the next major version.

I was confused about this because I had checked that the oldest version of vector you supported still had an IsList instance, so it must still have existed, but it turns out that back then it lived in GHC.Exts instead. GHC.Exts still re-exports IsList, so I've switched to using that. (Still happy to just give up and switch to loosening the dependency if you prefer.)

Not sure what your policy is on keeping old GHC versions in there.

We don't have a well-defined policy when it comes to supporting old GHC versions, or older stack resolvers, etc. I would be curious to hear, as a member of the Haskell community, what your preferred support policy would be for a library like ours?

To be honest I'm not much in contact with the mainstream Haskell community these days, so I won't be able to represent modern best practices or anything. I use LaunchDarkly at my day job, at which we generally only care about one GHC version at a time, which is currently GHC 9.2 but we hope to switch to GHC 9.4.

GHC itself lists the last three major versions (currently 9.8, 9.6 and 9.4) as "stable", so that's sometimes a heuristic that I've used. On the other hand, it seems like Debian stable (= bookworm) packages 9.0.x and so does Arch albeit in the latter case it is marked as "outdated". I haven't done a comprehensive survey of packaged versions at other distros.

I guess a relevant question is: can people on older compilers just use older SDK versions? If they still work (and empirically they do, we haven't upgraded to library version 4 yet) and you don't too often need to do mandatory security fixes, then maybe it's OK to say to people, you can still use our service but can't have the latest features until you upgrade your GHC.

@keelerm84 keelerm84 changed the title build: Drop explicit dependency on vector (to allow building with vector-0.13) fix(deps): Drop explicit dependency on vector Feb 1, 2024
@keelerm84
Copy link
Member

Thank you @bmillwood for this contribution, for the modifications, and for the insight into your needs as a user of this SDK.

@keelerm84 keelerm84 merged commit 3bb826d into launchdarkly:main Feb 1, 2024
9 checks passed
@bmillwood bmillwood deleted the no-explicit-vector-dependency branch February 1, 2024 19:44
@bmillwood
Copy link
Contributor Author

Thanks for fixing my missed renames :)

keelerm84 pushed a commit that referenced this pull request Feb 1, 2024
🤖 I have created a release *beep* *boop*
---


##
[4.0.2](4.0.1...4.0.2)
(2024-02-01)


### Bug Fixes

* **deps:** Drop explicit dependency on vector
([#69](#69))
([3bb826d](3bb826d))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: LaunchDarklyReleaseBot <LaunchDarklyReleaseBot@launchdarkly.com>
@keelerm84
Copy link
Member

@bmillwood this has been released in v4.0.2

@bmillwood
Copy link
Contributor Author

@bmillwood this has been released in v4.0.2

Thanks! I don't see a 4.0.2 on Hackage though?

@keelerm84
Copy link
Member

Sorry about that. It should be on there now.

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.

None yet

2 participants