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

Refactor: Create wire-api package for types used in the public API #1090

Merged
merged 160 commits into from
May 12, 2020

Conversation

mheinzel
Copy link
Contributor

@mheinzel mheinzel commented May 6, 2020

See https://github.com/zinfra/backend-issues/issues/1416.

This allows a clearer separation between the public interface and internal data structures. This is becoming more important with the upcoming federation changes, which will change internal representations while keeping a backwards-compatible API.

Things coming with this PR:

  • copied all types used in the interface/signature of public endpoints into a new wire-api package (mostly client API, but also some bot/provider stuff)
  • split long modules into smaller, more organized ones with a clearer structure
  • explicit export lists
  • Swagger models and JSON instances are next to the respective type definitions, making it easier to keep them in sync

I do not intend to change any definitions, just move them around (+ some formatting). This is supposed to be a pure refactoring.

Steps needed before this can be merged:

  • make wire-api tests compile
  • check in wire-api.cabal file
  • remove stuff from *-types that I copied over, still (re-)exporting it (to avoid diffs in import sites)
  • fix one TODO about which typeclass instance deriving strategy to use
  • rebase/merge develop, resolving conflicts

Cleanup I would like to do if there is time:

  • remove unused language extensions (introduced by copy-paste)
  • remove unnecessary package dependencies I introduced
  • add StrictData and remove all strictness annotations in types
  • make use of file "separators" consistent (lines made out of dashes)

@mheinzel mheinzel force-pushed the mheinzel/wire-api branch from 054abd4 to 4dbd75a Compare May 11, 2020 15:11
@mheinzel mheinzel marked this pull request as ready for review May 11, 2020 16:25
@mheinzel mheinzel changed the title [WIP] Refactor: Create wire-api package for types used in the public API Refactor: Create wire-api package for types used in the public API May 11, 2020
@fisx
Copy link
Contributor

fisx commented May 12, 2020

add StrictData and remove all strictness annotations in types

That i would have liked to see in a separate PR, but it's certainly a good change.

Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't read the whole thing, but I am reasonably confident this is good to go, except for some small things.

Also, two questions about the intended structure (not arguing for more change in this PR):

  1. So what is the purpose of types-common in contrast to wire-api? Why aren't they just one package?
  2. And what is the purpose of *-types packages in the future? Can they be resolved into types-common or wire-api? If not, how are they conceptually separate?

libs/galley-types/test/unit/Test/Galley/Types.hs Outdated Show resolved Hide resolved
libs/wire-api/src/Wire/API/Arbitrary.hs Outdated Show resolved Hide resolved
services/brig/src/Brig/API/Public.hs Show resolved Hide resolved
libs/wire-api/test/unit/Test/Wire/API/Roundtrip.hs Outdated Show resolved Hide resolved
services/galley/src/Galley/API/Public.hs Show resolved Hide resolved
@mheinzel mheinzel merged commit 4250ede into develop May 12, 2020
@mheinzel mheinzel deleted the mheinzel/wire-api branch May 12, 2020 14:38
@mheinzel
Copy link
Contributor Author

mheinzel commented May 12, 2020

Good questions.

  1. So what is the purpose of types-common in contrast to wire-api? Why aren't they just one package?

Right now, as we are depending on types-common, it needs to be considered part of the public API. In the future, I would like to get rid of some dependencies and move the rest into wire-api, but I think that's still up for debate.

  1. And what is the purpose of *-types packages in the future? Can they be resolved into types-common or wire-api? If not, how are they conceptually separate?

At the moment their purpose is a bit unclear and they mostly exist in their current form to avoid changing too much code at once.

For the future: Types used in the public API belong in wire-api and completely service-internal types could be moved into the services, so what remains would be types used for internal and inter-service endpoints. Maybe a name galley-api etc. would be more fitting then.

@fisx
Copy link
Contributor

fisx commented May 12, 2020

  1. And what is the purpose of *-types packages in the future? Can they be resolved into types-common or wire-api? If not, how are they conceptually separate?

At the moment their purpose is a bit unclear and they mostly exist in their current form to avoid changing too much code at once.

For the future: Types used in the public API belong in wire-api and completely service-internal types could be moved into the services, so what remains would be types used for internal and inter-service endpoints. Maybe a name galley-api etc. would be more fitting then.

I still prefer to add interal apis to wire-api in Intra-modules (it's a bit different from the Internal modules found in many libraries).

Otherwise 👍

@akshaymankar akshaymankar mentioned this pull request May 15, 2020
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.

3 participants