frasertweedale / hs-jose

Haskell JOSE and JWT library
http://hackage.haskell.org/package/jose
Apache License 2.0
122 stars 46 forks source link

Moving away from `lens` to provide classy optics #101

Open Kleidukos opened 3 years ago

Kleidukos commented 3 years ago

After a discussion with @frasertweedale, it has been decided to explore available options to end the dependency on lens for the API, while still retaining the classy optics that are not moving anywhere soon. There is also a plan to get rid of TH for optics generation.

Points to discuss:

frasertweedale commented 3 years ago

@Kleidukos thanks for filing. I'm at end of my day but will return tomorrow and set out all the API breaks I can think of, for discussion on how to proceed, and some rough ideas about the order in which to do things.

frasertweedale commented 3 years ago

@Kleidukos sorry for the delay, I should manage to get to it in the next couple days.

frasertweedale commented 3 years ago

OK, sorry it took so long. Rough plan:

  1. Define lens-compatible optic type synonyms and helper functions in new module Crypto.JOSE.Lens. The module can be exposed. It's fine to only implement what jose actually uses.
  2. In the library only, replace uses of the lens types, synonyms and functions with our own. But do not change the definitions of any of the optics until later steps. Also defer Control.Lens.Cons stuff for a later step.
  3. Rip out all the lens TH and hand-roll the classes/instances/accessors. This will be the most toilsome part.
  4. For types using Cons or AsEmpty, convert as suggested below (feel free to discuss). Because this is a breaking API change, the tests and example program may need updates too.
  5. Update definitions of other optics, if any.
  6. Last of all (and optional) remove use of lens from the test suite and example program, replacing with the values defined in jose.

Commits should follow the plan above, more or less.

Cons and AsEmpty conversions:

Note that use of lazy ByteString where the payload is concerned does not match with how the JWS is defined. Specifically, the payload is stored as a Types.Base64Octets, which is a newtype wrapper around strict ByteString. We can deal with this mismatch now (e.g. by defining a different newtype for lazy bytestring or abstracting Base64Octets over the representation. Or feel free to punt on it and just do the conversions wherever necessary, and I will follow up later.

I probably missed some stuff and we will no doubt have more to discuss as the work gets tacked.

Kleidukos commented 2 years ago

@frasertweedale I can't say I'm the most comfortable with the codebase, but if you're available for some pairing one day, I would love that. :)