Skip to content

Conversation

fosskers
Copy link

Motivation

General

lens is a very heavy dependency. This PR moves wreq off of lens and onto microlens, a performant subset of lens which has few dependencies and covers many use cases.

Specific

aura (a package manager) uses aur which uses wreq to access the Arch Linux AUR api. aura's executable size would be significantly reduced if the full lens dependency were removed.

@bos , please let me know if you have no intention of merging this, and I will fork the project.

TODO

  • Rip out lens
  • lens-aeson -> microlens-aeson
  • Use latest microlens and microlens-contra
  • Get tests to run
  • Include changes in recent commits
  • Update examples

- This was a reduction I made to clean up code, but I'm changing it back,
  as I'm going to attempt to have this merged into `wreq` proper before
  I bother forking it.
@bos
Copy link
Collaborator

bos commented Dec 22, 2015

Unless I am mistaken, this will break every existing user of wreq?

@fosskers
Copy link
Author

It's a drop-in replacement.

Elaboration: I'm currently converting Data.Aeson.Lens to run off of microlens as well. Users would just need to switch dependencies, some imports, and functionality remains the same.

@fosskers
Copy link
Author

Just discovered: the former _CookieJar Iso will need an explicit reverse-direction Lens. Luckily it's the only Iso in the code base.

@neongreen
Copy link

@fosskers How are you going to deal with responseLink and responseCookie being Folds? microlens doesn't provide Fold (because of the contravariant dependency).

Also,

Users would just need to switch dependencies, some imports, and functionality remains the same.

I don't think they would – lens is a strict superset of microlens. They might want to (in order to get rid of the lens dependency), but they wouldn't have to.

@neongreen
Copy link

Oh, I see, you used Fold from microlens-th. That's potentially not nice (“potentially” because functions operating specifically on Fold are rare and it might turn out that there won't be a single user affected).

@neongreen
Copy link

Here are the dependencies we still get rid of by switching from lens to microlens+contravariant:

lens kan-extensions parallel prelude-extras free
profunctors semigroupoids adjunctions distributive
base-orphans reflection bifunctors comonad

I think adding a contravariant dependency to wreq would be better than spreading fake folds around.

@neongreen
Copy link

@fosskers FYI, since microlens-0.3.5 Fold is available in Lens.Micro.Extras, so you don't need to pull microlens-th in.

@fosskers
Copy link
Author

Thanks @neongreen. We'd need microlens-th anyway for the makeLenses macro that is used in wreq, unless we opt to rewrite them all manually.

@fosskers
Copy link
Author

What is wrong with the Fold from microlens-th, exactly? (honest question)

@neongreen
Copy link

@fosskers

This is Fold from lens:

type Fold s a =
  forall f. (Contravariant f, Applicative f) => (a -> f a) -> s -> f s

This is Fold from microlens:

type Fold s a = forall r. Applicative (Const r) => Getting r s a
-- equivalent to
type Fold s a = 
  forall r. Applicative (Const r) => (a -> Const r a) -> s -> Const r s

We can't put a Contravariant constraint on f, so we just specialise f to Const r. This means that you can't use some functions on fake folds, such as foldByOf, foldMapByOf, and, as I just discovered, backwards and takingWhile. Losing the last 2 ones is especially unsettling:

> [1..5] ^.. backwards folded
[5,4,3,2,1]
> [1..5] ^.. backwards Lens.Micro.folded

<interactive>:
    Couldn't match type Control.Applicative.Backwards.Backwards
                           (Const (Data.Monoid.Endo [t])) t
                   with Const r0 t
    Expected type: Optical
                     (->)
                     (->)
                     (Control.Applicative.Backwards.Backwards
                        (Const (Data.Monoid.Endo [t])))
                     [t]
                     [t]
                     t
                     t
      Actual type: Lens.Micro.Type.Getting r0 [t] t    

Actually, it's so unsettling that I'm probably going to remove Fold altogether in the next major release.

@fosskers
Copy link
Author

I'm going to cross that bridge once microlens-aeson is done. Could responseLink and responseCookie work anyway with the fake Fold? The wreq test suite should tell us.

@neongreen
Copy link

Even if the test suite doesn't fail, there still might be code in the wild that would refuse to compile once you switch responseLink and responseCookie from being lens folds to being microlens folds.

@fosskers
Copy link
Author

If the user was doing something very Fold specific with those values.

@fosskers
Copy link
Author

With the advent of SimpleGetter and SimpleFold in microlens, this will probably need to be its own fork, as lens and microlens diverge there in functionality.

@neongreen
Copy link

@fosskers

If the user was doing something very Fold specific with those values.

I wouldn't call backwards very Fold-specific – it's a general-purpose combinator that works (and is useful) for pretty much all lens-like things.

lens and microlens diverge there in functionality

They already diverged when Fold was introduced, the renaming just makes it clearer and warns people who see a fake Fold in a library that something fishy is going on there. When Lens.Micro.Extras exports a less powerful view, it's your decision to use it, and nobody else is hurt. When you export a fake Fold, everyone is (potentially) hurt.

The reason SimpleFold exists at all is that without it people would be exporting folds anyway, but those folds wouldn't be marked with a convenient type synonym (or, even worse, people would just define the Fold type synonym by themselves). There's no good solution here:

  1. either I introduce new names that aren't in lens and make it sli-ightly harder for people to switch from microlens to lens
  2. or I don't introduce new names and people who use your library get cryptic errors they wouldn't know how to debug
  3. or I don't introduce any names and people start seeing signatures like Monoid m => Getting m s a that aren't descriptive and don't help debugging either

I went for 1) because I think it's less painful than the rest.

I also just pushed another microlens library that depends on contravariant and has true Fold – depending on it would be pretty painless for wreq, since contravariant void semigroups StateVar are pretty small packages by themselves and all their dependencies are already pulled in by wreq itself.

@reiddraper
Copy link

@fosskers if executable size is your main concern, have you tried compiling Aura and dependencies with -split-objs? I've seen this drop the executable size by an order of magnitude.

@fosskers
Copy link
Author

@reiddraper Most of my users prefer a standalone binary of aura. Unless I'm misunderstanding, -split-objs wouldn't allow that.

@fosskers
Copy link
Author

@neongreen In using microlens, people should be aware that they're not getting the "full package" so to speak (your READMEs make that quite clear), so having a custom Fold type shouldn't be that much of a surprise. The Fold usage in wreq will have to be converted to SimpleFold, regardless of the potentially reduced functionality.

@neongreen
Copy link

In using microlens, people should be aware that they're not getting the "full package" so to speak

Yep, sure. However, people who use wreq shouldn't care about whether wreq uses lens or microlens – and if you suddenly change exported folds to Lens.Micro.Fold (and the type signature doesn't even change visually), they would have to care.

@fosskers
Copy link
Author

I agree.

@reiddraper
Copy link

@fosskers you will get a full binary, just as you would w/out split objects. It will just be much smaller in size. As a tradeoff, compilation is slower.

@fosskers
Copy link
Author

@reiddraper Oh? Slower compilation I can deal with, since I'm footing that time on my side so that my users don't have to.

@fosskers
Copy link
Author

@neongreen Just took a look at microlens-contra. I'll very likely be using that and keeping the true Folds around.

- Although not all of them pass. Not the fault of `microlens`.
- As opposed to the "fake" `Fold` as provided in earlier versions.
@fosskers
Copy link
Author

fosskers commented Jan 4, 2016

This now uses the true Fold from microlens-contra.

@fosskers
Copy link
Author

@bos , this is ready. Note that I haven't attempted to fix the 3 test failures, as those should be a different PR and don't strictly have anything to do with microlens.

@fosskers
Copy link
Author

Any word, @bos ?

@reiddraper
Copy link

I am -1 on this. I think it gives little value to the average user of Wreq. At least from my perspective, one of the core ideas of Wreq was to use Lens, so I don't avoiding a Lens dependency is a big deal to most Wreq users.

@fosskers did you end up compiling aur with -split-objs? How much did it help with executable size?

@fosskers
Copy link
Author

Maybe not to most, but to this wreq user right here, it is.

I haven't, as I use stack for my builds, and they are currently discussing the best way to enable cabal config options (see commercialhaskell/stack#1438).

@fosskers
Copy link
Author

@bos Please let me know if you don't intend on merging this, and I'll make a fork. However, I do believe that keeping wreq as one project is best for the community.

@fosskers
Copy link
Author

fosskers commented Feb 2, 2016

Something to note: Since lenses are just type aliases, and wreq exports no Iso or Prism, users of wreq would not see a functionality break from this. microlens Lens and Traversal definitions are the same as those in lens.

@bdesham
Copy link
Contributor

bdesham commented Feb 18, 2018

@fosskers Did you ever create a full-fledged fork of the project?

@fosskers
Copy link
Author

I didn't no - I use servant-client for http requests now.

@fosskers fosskers closed this Nov 25, 2019
georgefst added a commit to georgefst/wreq that referenced this pull request Apr 15, 2022
…rpreter`)

As it stands, we also need changes to `entropy`, but I'll manage that separately. Hopefully by reviving haskell#78.

Note that the custom setup is only used for `cabal-doctest`, so we really can just safely remove it. This is essentially the same issue as cdepillabout/pretty-simple#82.

Seeing as all the TH is just for `makeLenses`, in an ideal world we really ought to just be able to run on the host: see ghc-proposals/ghc-proposals#243.

The changes to the two `.hs` files are from dumping the TH splices (and manually (well, with _some_ help from HLS) adding a load of qualified imports). The splices can be found in `dist-newstyle` after adding this to `cabal.project`:

```
package wreq
    ghc-options: -ddump-splices -dth-dec-file
```
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.

5 participants