New way of generating contracts

[jakubfijalkowski]

Consider this example of contracts (in terms of CoreLib). You can see there a single, static Auth class with some constants, ExampleCommand and both Dart & TS contracts. I want to start a discussion on how the contracts should look like (in both languages) and what are the current deficiencies there.

There are already some questions/bugs in the issues, but I assume there are more. Here are mine.

Flat contracts

The generated contracts are flat. Affects both TS & Dart contracts.

This is the main problem with them. Designing backend API without some form of a hierarchy is very problematic (which we've seen in multiple projects). It also makes for a rather unintuitive and hard to grasp client definition. You end up with hundreds of classes and methods in a single file - even tooling doesn't help that much with discoverability here. Especially if you have everything flat.

I don't really know how contracts with hierarchy preserved should look like, since both Dart and TS lacks namespaces. We could emulate that by using multiple files and just importing them where needed but that might be... not ergonomic. It might also not work in Dart (I'm not that fluent there).

The other option (TS-only I think) would be to just generate a one, big object with namespaces being fields. I think @konowrockis suggested that somewhere. Basically, the contracts would look like this:

const API = {
  LeanCode: {
    Example: {
      Core: {
        Contracts: {
          Auth : {
            Roles: {
                User: "user",
            },
            KnownClaims: {
                UserId: "sub",
                Role: "role",
            },
            Clients: {
                AdminApp: "admin_app",
                ClientApp: "client_app",
            },
            Scopes: {
                InternalApi: "internal_api",
            },
          },
          Example: {
            ExampleCommand: {
                ErrorCodes: {
                    EmptyArg: 1,
                },
            },
          },
        },
      },
    },
  },
};

Not best, but at least preserves the intention.

For Dart I think we would need to embed the namespace in the class names, e.g.:

class LeanCode_Example_Core_Contracts_Example_ExampleCommand implements IRemoteCommand {
    LeanCode_Example_Core_Contracts_Example_ExampleCommand();

    factory LeanCode_Example_Core_Contracts_Example_ExampleCommand.fromJson(Map<String, dynamic> json) => _$LeanCode_Example_Core_Contracts_Example_ExampleCommandFromJson(json);

    @JsonKey(name: 'Arg')
    String arg;

    @override
    String getFullName() => 'LeanCode.Example.Core.Contracts.Example.ExampleCommand';

    @override
    Map<String, dynamic> toJson() => _$LeanCode_Example_Core_Contracts_Example_ExampleCommandToJson(this);
}


@JsonSerializable()
class LeanCode_Example_Core_Contracts_Example_ExampleCommand_ErrorCodes {
    LeanCode_Example_Core_Contracts_Example_ExampleCommand_ErrorCodes();

    factory LeanCode_Example_Core_Contracts_Example_ExampleCommand_ErrorCodes.fromJson(Map<String, dynamic> json) => _$ELeanCode_Example_Core_Contracts_Example_ExampleCommand_ErrorCodesFromJson(json);

    static const emptyArg = 1;

    Map<String, dynamic> toJson() => _$LeanCode_Example_Core_Contracts_Example_ExampleCommand_ErrorCodesToJson(this);
}

The "long names" problem might not be that... problematic. IMO APIs like this should be isolated, so you would need to use the full name at most a few times in the whole project.

Attributes

This is also mentioned in the issues but I think it deserves more discussion. I would really love to see the attributes generated in the final contracts. Losing them prevents end-clients from seeing the whole picture.

Attributes might be used not only for authorization (currently that's the main usage) but I envision some form of documentation-in-the-code that represents missing relations or missing types (e.g. something like "this command corresponds to this event in the SignalR" or "this is step 5 of the order flow").

ErrorCodes

Currently, Dart and TS have completely different handling of error codes, to the point where Dart ErrorCodes classes are unusable.

TS is on the other side of the spectrum - ErrorCodes is a special class that must consist of const ints. It is then required to handle all the errors defined there if you want to call the endpoint. This is great, but the current implementation lacks flexibility - you can't compose the ErrorCodes classes (nor inherit them). The TS generator will work (generating code that loses the Photos error codes), but Dart's will blow up. Sample code:

[AllowUnauthorized]
public class ExampleCommand : IRemoteCommand
{
    public string Arg { get; set; }
    public List<PhotoDTO> Photos { get; set; }

    public static class ErrorCodes
    {
        public const int EmptyArg = 1;

        public sealed class Photos : PhotoDTO.ErrorCodes { }
    }
}

public class PhotoDTO
{
    public Uri Url { get; set; }

    public class ErrorCodes
    {
        public const int UrlInvalid = 7_001;
        public const int UrlDoesNotExist = 7_002;
    }
}

This is somewhat related to issue #103 .

---

What do you think? Do not constraint yourself with the current approach - we might (and probably should) rewrite big parts of the generator anyway, so everything is possible. Even using other IDL than C# (but we must have really convincing arguments here ;) ).

[mchudy]

I don’t really know how contracts with hierarchy preserved should look like since both Dart and TS lacks namespaces.

That’s not really true for TS and I believe that the quick POC by @konowrockis that you’re referring to was actually based on namespaces. Although namespaces vs JS objects is more of an implementation detail I think (I might be wrong and fail to see some ramifications of that choice at the moment).

Dart

As for Dart, that goes much deeper. First of all, the current Dart CQRS client is too low-level. This is how a typical call to the API looks like right now:

 final response = await _cqrs.run(
      c.SendMessage()
        ..messageId = messageId
        ..conversationId = conversationId
        ..content = content,
    );

• We are passing raw Command objects to run (and Query objects for get). If we wanted to know what commands and queries are available, we would need to look in contracts.dart. CQRS is actually a good low-level abstraction for the protocol layer, but we need some higher-level wrapper like AppClient in TS.
• We use public setters instead of a constructor which means we can omit and edit properties freely. Command and query objects should be immutable while respecting the optionality of arguments.
• CommandResult is not generic over the error codes type - errors is just an array of objects containing codes and messages. We have to manually match the code with the proper class from contracts. That is unproductive and prone to errors. Moreover, in TS we have nice tooling around it (handleResponse) which makes the whole error handling much less of a hassle. I don't know if it's possible with @JsonSerializable, maybe we could utilize freezed?

Those problems don’t exist in the TS client (although we could think of some improvements surely).

I think the call should look more like this:

 final response = await _apiClient.sendMessage(
    messageId: messageId,
    conversationId: conversationId,
    // say content is optional, we can omit it or allow null with null-safety
 );

Namespaces

In my view, things like LeanCode_Example_Core_Contracts_Example_ExampleCommand look like an ugly hack (unless they are to be a hidden part of the implementation, but still). I don't really get the ergonomics problem with multiple files. You say we need hierarchy, I say we need more - a physical separation (or more like a simulation of it). Putting everything into one huge client will ultimately be a mess (especially at scale - although we might never get into territories where that would really matter).

Let’s imagine the generator respected bounded contexts of the domain:

/contracts
  authApiClient.ts
  shippingApiClient.ts
  inventoryApiClient.ts

I know, we might use contracts-config.json to mimic it somehow, but that’s too much configuration and it’s not a great DX.

We get a nice scalability for free - different parts of the application could be extracted to separate packages/sub-applications:

/auth
  /contracts
    authApiClient.ts
  /components
  /store

We don’t break the interface segregation principle, we don’t need to care about name clashes (kind of, the case of SignalR events remains problematic), we have smaller clients, we can clearly set domain and ubiquitous language boundaries, we can introduce different versioning, we can easily see in diffs which changes affect which parts of the application. I might have got carried away here and it’s not a full-on solution for namespacing issues, but I think we should look more holistically at contracts.

Int he end, we could still put all those clients in a single contracts file if we wanted.

Attributes

That’s interesting, but I think we need to come up with some real-life cases for that before we get into implementation. For me, it would only bring significant value if there was some client-side tooling involved.

Authorization (unless it’s based on roles) is not a valid use case for that IMO. What can the client do with the AuthorizeWhenOwnsOrderAttribute attribute? Arguably, not much. I need to understand the domain to know what the ownership means and how can I check that condition to e.g. hide some button on the UI. I’ll never get to the level of calling the command anyway.

Using attributes for SignalR is an interesting idea and actually, maybe we should aim to have an auto-generated domain-specific SignalR client as well? I’ve never worked on a LeanCode TS app with SignalR, but in Dart we just get a bunch of DTOs scattered around - some are used by SignalR, some by the HTTP API. The only way to know for sure is to look into the backend code. Which is bad, and Hungarian notation in the form of prefixes/suffixes (albeit better) similarly so.

[Albert221]

I like the idea of having the contracts generator output more high-level stuff like the apiClient @mchudy showed. This could be an additionally generated abstraction generated on top of the contracts themselves.

I wrote some example code on what could be generated. Of course, the naming, how and when to preserve namespaces and how (and if) to split it into multiple files are subject to discussion, but having such an abstraction would defeat the need of using the Query and Command objects directly.

// Given we have a CreateUser command inside LeanCode.Example.Core.Contracts.Users
// And a FetchOngoingPayments query in LeanCode.Example.Core.Contracts.Users.Payments

class ClientApi {
  ClientApi(this._cqrs) : users = ClientUsersApi(_cqrs);

  final CQRS _cqrs;

  final ClientUsersApi users;
}

class ClientUsersApi {
  ClientUsersApi(this._cqrs) : payments = ClientUsersPaymentsApi(_cqrs);

  final CQRS _cqrs;

  final ClientUsersPaymentsApi payments;

  Future<CommandResult> createUser(String email, String password) {
    // Pass it directly to the constructor which would actually
    // make more sense and be more error-proof thanks to the analyzer
    // and required/optional constructor params.
    return _cqrs.run(Users_CreateUser(
      email: email,
      password: password,
    ));
  }
}

class ClientUsersPaymentsApi {
  ClientUsersPaymentsApi(this._cqsr);

  final CQRS _cqrs;

  Future<List<PaymentDTO>> fetchOngoingPayments() {
    return _cqrs.get(
      Users_Payments_FetchOngoingPayments(),
    );
  }
}

void main() async {
  final cqrs = CQRS();
  final clientApi = ClientApi(cqrs);

  final result = await clientApi.users.createUser('email', 'password');

  final payments = await clientApi.users.payments.fetchOngoingPayments();
}

IMHO the only namespace part that should be crucial to generation should be the part relative to LeanCode.Example.Core.Contracts (somehow derived or specified directly in contracts-config.json) so that there would never be a LeanCode_Example_Core_Contracts_Users_CreateUser but a Users_CreateUser.

---

Summing up, I second:

• passing query/command arguments through constructor, not fields
• generating a higher abstraction over contracts (which would introduce a hierarchy)
• having generic CommandResult<ErrorCode> is a good idea, but worth a separate discussion with the mobile team
• simplifying the contracts-config.json a bit (I'm looking at you TypeTranslations)

A propos attributes, I personally don't see any real-world use case for them on the frontends. Not with how our current Dart contracts-related ecosystem works.

[konowrockis]

First of all I have a feeling that the general line that this issue follows is to make TS and Dart generators behave exactly the same. My question is: why? Those two languages are so different that it's super hard to compare them and if we limit ourselves to treating them as one we won't benefit any of them. Saying that IMO any further low-level discussions should be treated as completely separate issues for Dart and TS. Of course high-level issues like flattiness, attributes etc. apply equally to both of them but low-level like namespacing or error handling IMO don't.

Flat contracts

Currently I'm working on the project where I don't have our generator 😥 While the tech stack closely resembles ours I needed to come up with some way of defining the contract. On frontend we already try to divide our residual domain into some boundaries. I followed that path and defined (manually) contracts for each of these boundaries. The benefit of that approach is that each of those boundaries is quite small so the contract file won't grow uncontrollably (if it does maybe it's good idea to divide). Also it improves discoverability because you know when you can expect given things and often you don't work on two boundaries at once.
To the point, I think dividing contracts is really good idea. It makes them more intuitive and easier to cope with. However:

1. We can have that. Generator currently supports filtering in files by regex. @mchudy mentioned that it's too much configuration and bad DX. Maybe what we need is to somehow rethink how we configure things? Maybe we only need to have some shared parts of config? This solution has an issue that the backend decides how things are divided. I guess in 99% of the cases this is sufficient. If not then we maybe should cooperate closer between frontend and backend rather than findinig an infrastructural solution? This also has added benefit of resolving issues with name clashes (if done correctly).
2. I think if we want to have flexibility and make generated structure independent of backend structure it will require more manual steps somewhere in the process (regardless the process).

Attributes

I kinda agree with @mchudy on this point. I feel that attributes are internal backend stuff. Even @Albert221 in his original issue says that:

I don't see any value in having those WhenOwnsAuctionOrDoesNotExistAttribute and WhenOwnsAuctionOrDoesNotExist classes generated.

So I'm not exactly sure what do we want to achive here with attributes.
Maybe we want to transfer documentation comments? That sound like much better idea.

[jakubfijalkowski]

Okay, one by one:

@mchudy

That’s not really true for TS and I believe that the quick POC by @konowrockis that you’re referring to was actually based on namespaces.

I stand corrected. They might be useful.

Dart

CQRS is actually a good low-level abstraction for the protocol layer, but we need some higher-level wrapper like AppClient in TS.

Agreed, except that if we want to have correct abstraction of the protocol, the DTOs cannot be "flat". We might have conflicting names of DTOs and/or queries/commands across different namespaces (this is the one thing that I won't be giving away), and your lowest-level client needs to handle that correctly. If higher-level clients are auto-generated, then these should also take that into account.

We use public setters instead of a constructor which means we can omit and edit properties freely

I think this is OK for low-level layer, but it tends to be problematic if that is the only layer. We started doing extensive validation because of that, but even with it, it introduces unnecessary back-and-forths for developers.

CommandResult is not generic over the error codes type - errors is just an array of objects containing codes and messages.

That's exactly what I had in mind - you lose the connection between command and possible error codes in Dart. This isn't terrible, but introduces friction (and unhandled errors).

Namespaces

In my view, things like LeanCode_Example_Core_Contracts_Example_ExampleCommand look like an ugly hack (unless they are to be a hidden part of the implementation, but still

Indeed, these are. I don't know Dart enough to come up with anything more meaningful.

Let’s imagine the generator respected bounded contexts of the domain:

Let's not conflate bounded contexts with APIs. A single bounded context might have multiple APIs (and I don't think this is bad), like "end-user + admin" or "contests + rankings" (in Activy "Contests" bounded context is split between two apps and two, semi-related APIs).

Nevertheless, that might be not enough. We might introduce internal rule that says we don't have name collisions across a single API but then everyone needs to respect that, and it might result in too granular APIs. This is, IMO, the best option for now.

Putting everything into one huge client will ultimately be a mess (especially at scale - although we might never get into territories where that would really matter).

Indeed, but generating "separate file for separate something" requires clear definition of something or you will end up with hundreds of small files with very convoluted imports (or even circular imports - this happens).

Attributes

Authorization (unless it’s based on roles) is not a valid use case for that IMO. Arguably, not much

I disagree. You don't need to use the attributes but you should be aware of them. Treat them like a documentation - it will remove the need to go to C# contracts when 403 Forbidden happens. You don't need client-side tooling for that.

I need to understand the domain to know what the ownership means.

And you should know that already - you need to know where it is applied.

Using attributes for SignalR is an interesting idea and actually, maybe we should aim to have an auto-generated domain-specific SignalR client as well.

I don't think we use SignalR that much. Just documentation would suffice.

@Albert221

Thanks for the code! It really helps getting grasp on how you might want the client to look like.

IMHO the only namespace part that should be crucial to generation should be the part relative to LeanCode.Example.Core.Contracts.

That's a very good idea (and should be easily achievable), although I have a question - does Dart allow name clashes across different files (i.e. can you have CreateUser in two different files that don't reference each other nor are used together)?

simplifying the contracts-config.json a bit (I'm looking at you TypeTranslations)

Can you elaborate? I didn't think it was problematic. I had more problems with the regexes than with this.

A propos attributes, I personally don't see any real-world use case for them on the frontends.

Treat them like a documentation, not like "programmable attribute".

@konowrockis

First of all I have a feeling that the general line that this issue follows is to make TS and Dart generators behave exactly the same. My question is: why?

That's wrong impression. I don't want them to behave the same, but if we want to come up on some limitations on how we structure contracts, then both parties must agree. The language-dependent clients might differ but the source contracts must be the same.

Of course high-level issues like flattiness, attributes etc. apply equally to both of them but low-level like namespacing or error handling IMO don't.

Consider error handling - if we have ErrorCodes as a first class citizen, then I (or in other words - the writer of contracts) expect that it suffices for both. If one handles it but the other does not, then I will need to create them twice (or say them "over the shoulder" or send link to the class). That's unnecessary and error-prone work.

Also, having two completely separate generators (if the end-clients differ substantially) will introduce much more work for us. It's better to at least try to minimize the work.

Flat contracts

To the point, I think dividing contracts is really good idea. It makes them more intuitive and easier to cope with.

I agree. If so, we just need to define how granular the parts should be ('cause "bounded context" is too much, as I said above) and how preserve hierarchy there (at least to some point).

Maybe what we need is to somehow rethink how we configure things?

We should, but that alone will not be sufficient - if the configuration does not work with how we structure contracts, then we will have problems. :P

Maybe we only need to have some shared parts of config?

What do you mean? Separate config for Dart and for TS? From my experience, these will diverge quickly which might be not effective overall. I don't think we need to have feature (and config) parity, but trying to do everything in both generators will be better overall. Only if that's not possible or not needed, separation should be considered.

This solution has an issue that the backend decides how things are divided. I guess in 99% of the cases this is sufficient. If not then we maybe should cooperate closer between frontend and backend rather than findinig an infrastructural solution?

That's not infrastructural solution IMO - this is a way to start the cooperation, on a lower level. This way we might have a "base" for future discussion, 'cause right now we can't even cooperate because people don't know where to start (at least that's my impression).

This also has added benefit of resolving issues with name clashes (if done correctly).

How? Cooperation only won't really work here - UserById is a valid, and sometimes the only sensible, solution in many cases, even in the same "context".

Attributes

I feel that attributes are internal backend stuff.

I disagree. They are used by backend, but also define how the API behaves. And this behavior is, most of the time, crucial for proper usage of the API.

Real story - QueryCache. We used it quite extensively in one of the projects, except that the client was not aware of the caching. To create the feature correctly, the client-app team had to refresh the cache at correct moments. Except that it was missed (since it haven't shown up in the contracts) and we had to iterate over the feature twice or thrice to make it work for all. To the point, where the caching was lifted because it showed problem long after the app was deployed. If only more people looked at it earlier, we would be able to prevent all these problems (by either removing caching since it was not needed or by acknowledging existence).

So I'm not exactly sure what do we want to achive here with attributes.

Propagate knowledge and proper definitions. Authorization is part of the API spec, you don't need to use it explicitly, you just need to know about it (and not guess). Treat it like documentation.

Maybe we want to transfer documentation comments? That sound like much better idea.

Except that comments will go outdated sooner than attributes.

---

Regarding the attributes - I have the impression that you think that my idea is to use them by the client. This is not true. The only thing I want to achieve is to communicate better, without doing all the back-and-forth discussions. I will be more than happy to generate attributes as comments (but then it's better not to make exceptions, like #194 ) if they will be visible to the end-user.

[mchudy]

C# has its own DSL for comments (much more verbose than Dartdoc), so if we want to adhere to standards on all three platforms that means having additional work with transpilation.

/// <summary>This method changes the point's location by
///    the given x- and y-offsets.
/// <example>For example:
/// <code>
///    Point p = new Point(3,5);
///    p.Translate(-1,3);
/// </code>
/// results in <c>p</c>'s having the value (2,8).
/// </example>
/// </summary>

public void Translate(int xor, int yor) {
    X += xor;
    Y += yor;
}   

[mchudy]

Heh, I see we went to the same site for example :D

[Albert221]

Ah. Sorry. I understood that we could preserve the attributes (e.g. those related to authorization and caching) but only as comments and not actual language construct like annotations.

In this case, we may want to either (despite the transpiling)

• stripe the documentation comments from tags
• only replace stuff like <c> with the grave accent for Markdown <code>
• just leave them as is, with those XML tags.

[jakubfijalkowski]

I feel like you're trying to have auto-generated documentation that would bring some valuable insights - that's unrealistic

You reject auto-generated documentation as "unrealistic" and I tend to disagree with that. That's nowhere near "unrealistic". Having "auto-generated documentation" as the sole source of truth is unrealistic and I don't really want that. Attributes, the way they are used in our contracts are form of documentation (and not autogenerated! They are written manually and I just want to preserve the meaning), except they live with code as opposed to comments (or documentation) that live next to code and isn't always fresh. Attributes at least needs to be maintained by the backend developers (otherwise the API won't work because it will either throw in case of improper authorization, or plainly won't compile if you used wrong types).

All the "API" standards use exactly that approach - GraphQL (you don't define schema, it gets generated from the underlying data source, using types), Swagger (or more generally - OpenAPI generated from frameworks) - they all base documentation on real code. Even code samples in Haskell or Rust that are executed by tools are form of "attributes" (because you give them higher level meaning) because they need to preserve some imposed form.

The rules are too complex for that (and as I mentioned, you have to be familiar with the whole domain, roles, permissions)

You should know your domain and everything related to it in the first place. Saying that "I don't know what "owns the order" means" isn't an excuse.

Backend is the final source of truth of how authorization actually works, but it won't replace or substitute the whole information flow within the project team.

Indeed, it is. And it uses attributes to describe that. Only attributes, nothing more. You won't get "403 Forbidden" from anything there. You don't need to know how it is implemented. AuthorizeWhen attributes tend to be rather easy to understand ("owns", being the most popular one is, most of the time, used in the project in other places).

I don't want to replace communication, but the "information flow" you're describing tends to be "let's ask our backend dev who we need to be to run this command because someone forget to put that in the US or assumed it is evident" and I'm quite fed up with answering the same question for the hundredth time.

why not use Cache-Control

Because proper Cache-Control is too complex for most of our needs (and it is implementation detail on how you implement it on the transport layer). And this also does not address my main point, as you said - you still have to handle it explicitly. But you don't even know that it is used in the first place.

Also, exposing HTTP primitives on higher-level layers defies the point of abstraction that we have here.

Typically, when I'm integrating with some command I have it all described in the user story

This is unrealistic. It is frequent that the "environment" is not described in the US and you only find out about it when you start coding.

Thus, custom comments on non-trivial fields would be more useful.

Except you're trading "free form text that is not checked by anything" with "almost free-form text that is at least checked by one side".

Nevertheless, let's stop the attributes discussion here. We're getting nowhere.

[jakubfijalkowski]

@Albert221

only replace stuff like <c> with the grave accent for Markdown <code>

This option doesn't scale (and we, meaning backend devs, tend to ignore this syntax and use "`" nevertheless) because there are is non-negligible trickery to handle that properly.

stripe the documentation comments from tags
just leave them as is, with those XML tags.

These two are the only options I think. The first one might be quite tricky (you might strip too much) and the latter one might be overly expressive, but we can come up with a sensible algorithm here.

[Albert221]

That's a very good idea (and should be easily achievable), although I have a question - does Dart allow name clashes across different files (i.e. can you have CreateUser in two different files that don't reference each other nor are used together)?

Yes! It's perfectly fine. If you want to import two files that both have CreateUser class, you have few options:

• use import 'a.dart' hide CreateUser; to hide the class from the file if you don't use it
• use import 'a.dart' show SomethingIActuallyNeed; to only expose the class you need and not other conflicting classes
• (the best one) use import alias import 'a.dart' as a; and then use both classes a.CurrentUser

simplifying the contracts-config.json a bit (I'm looking at you TypeTranslations)

Can you elaborate? I didn't think it was problematic. I had more problems with the regexes than with this.

It would be nice to have a good default value for that so we wouldn't have to explicitly provide TypeTranslations in every project. Also, I think this should be merged with defaults (so letting the defaults stay, possibly overriding some of them) instead of forcing the developer to provide every type again.

I will be more than happy to generate attributes as comments (but then it's better not to make exceptions, like #194)

The Obsolete/Deprecated is actually something that gives a real feedback to the developer as they write the code. I am strongly standing by my point in #194.

[konowrockis]

This example kinda shows that it needs to be addressed per-attribute.

[jakubfijalkowski]

Okay, so I feel that I will be alone in the pursuit of "don't lose anything in the translation". :(

So, from now on, let's assume that attributes are out-of-scope of being generated in the client, and if the need arises (as with Obsolete/Deprecated), we will tackle them one by one.

[jakubfijalkowski]

It would be nice to have a good default value for that so we wouldn't have to explicitly provide TypeTranslations in every project.

Ah, I see. And strongly agree.

[konowrockis]

I think for frontend making better defaults is not really possible. The main reason why TypeTrasnaltions diverges in pretty much every project is date handling. We have no native way to conveniently express dates so every project kinda does it on its own. Much better approach is the one mentioned somewhere in the conversation with overwriting defaults rather than replacing. Also I think that having shared parts of config could also address this issue somehow.

[jakubfijalkowski]

I think for frontend making better defaults is not really possible.

By "defaults" I meant the merging strategy Albert described.

[jakubfijalkowski]

To sum things up:

1. Attributes are a no-go apart from Obsolete that will be generated (see Generate @Deprecated() annotations for [Obsolete] structures #194),
2. We need to generate/copy comments to the resulting contracts,
3. We need to introduce some hierarchy, but the hierarchy shouldn't really reflect namespaces (should it?) in the contracts,
4. It's up to mobile team to come up with some solution to ErrorCodes - should we even care?
5. We need to rework the configuration model.

Hierarchy

We don't have any definite answer on how we want the hierarchy to look like. On one hand, I would prefer not to have to strict rules here, on the other hand - making it too flexible might be abused. I have really two ideas that might be combined, so well... three proposals.

Manual

In the config file we have something like this:

{
  "clients": [
    {
      "name": "admin",
      "sub": [
        {
          "name": "projects",
          "sub": [
            { "name": "mgmt", "namespace": "LeanCode.Example.Core.Contracts.Admin.Projects.Management" },
            { "name": "reports", "namespace": "LeanCode.Example.Core.Contracts.Admin.Projects.Reports" }
          ]
        },
        { "name": "users", "namespace": "LeanCode.Example.Core.Contracts.Admin.Users" }
      ]
    },
    { "name": "user", "namespace": "Leancode.Example.Core.Contracts.User" }
  ]
}

So basically you define the clients explicitly.

Pros:

• Full flexibility,
• Shouldn't be hard to implement,
• Might fluently adapt to the project,

Cons:

• Needs to be defined manually,
• Doesn't easily allow "shared" DTOs,
• At a single level you can have either sub-clients or real contracts, otherwise we need to come up with a rule that describes when to generate recursively and when not.

Folder structure

We can also define the clients in terms of the folders. We might say that hierarchy will be two levels deep and then use folders to preserve that. For example

• Admin
  • Projects
    • Management
    • Reports
  • Users
• User
  • Profile

and depth = 2 will result in following clients:

• admin
  • projects
  • users
• user
  • profile

Pros:

• Easy configuration,
• Really forces you to think how you structure the contracts.

Cons:

• Not flexible at all,
• Might require artificial folders (e.g. user.profile - we don't have anything else on profile level),
• Doesn't really allow mixing level types - the same problem as with the previous solution,
• Doesn't easily allow "shared" DTOs.

The mix

We can mix the solutions, i.e. in the first one, instead of manually defining all the clients, we can have a third entry type, depth, that works the same as the above solution but starts the scan at the specified namespace. Has the same advantages as the above solutions combined, but some cons just don't hold. Additionally, it is the hardest to implement.

These aren't the only solutions (hopefully). Do you see something better? Maybe you have ideas on how to fix the "cons"?

---

Client look-and-feel

We need to come up with the look of end-clients, assuming that they are hierarchical. There is some work done already by @Albert221 for the Dart one, but it needs polishing (and precision). May I request your help here @Albert221, @konowrockis, @mchudy? Can you come up with the look of the clients, considering the aforementioned requirements (and all the discussions here)?

[mateuszwojtczak]

Why the second one doesn't easily allow shared DTOs? Wouldn't there be a directory named e.g. shared for those? And is it hard to make the dependencies work? E.g. one file importing shared contracts file?

[jakubfijalkowski]

That is slight oversimplification. Having shared folder somewhere in the tree requires you to resolve cross-NS dependencies during first phase (parsing contracts) which isn't easy (AFAICR we don't need that right now - we just rely on syntactic names) as you must incorporate full dependency resolution. What is more, you then have to have a way to resolve the dependencies in the generated clients which introduces even more complexity - you have to know what you need to import (or impose additional restrictions), where to import it from, and then mix that with language-specific behaviors.

And then you need to decide how to configure that and how you want to enforce what can be shared and what cannot.

I'm not saying that is not doable, but with both approaches it needs to be explicitly modeled.

[mchudy]

It's true that supporting shared libraries will add a lot of complexity to the parsing process, but when it comes to the sole code generation supporting shared libraries doesn’t seem like that much of an issue to me. Every client could import shared files at the top of the file. We would require that shared parts cannot have dependencies on other parts (including other shared projects), which seems like a reasonable assumption. The generated code could look like this: (assuming we want multiple shared projects, but I think it doesn’t make a big difference and we gain some elasticity)

import '../shared1.dart' as shared1;
import '../shared2.dart' as shared2;

import * as shared1 from '../shared1';
import * as shared2 from '../shared2';

This will involve some extra path trickery in the implementation. We could also use absolute imports.

In the manual approach shared libraries could be specified alongside clients, so we could have something like this:

{
  "shared": [
    { 
	"name": "shared",
	"namespace": "Leancode.Example.Core.Contracts.Shared",
    }
  ],
  "clients": [
    { 
	"name": "user",
	"namespace": "Leancode.Example.Core.Contracts.User",
    }
  ]
}

For the second approach, we could use a convention analogous to the one used for clients.

There’s one other thing though that I feel we’re missing in the discussion. What should the output look like (structure-wise)? I guess we want to have one file per client?

In the manual approach I guess we could add (possibly optional) name property which would specify the output folder:

{
  "clients": [
    { 
	"name": "user",
	"namespace": "Leancode.Example.Core.Contracts.User",
        "path": "src/user/contracts",
   }
  ]
}

This of course complicates the implementation for shared libraries. We could also use one top-level path and don't allow for configuration on a client level basis.

For the convention-based approach, we would have to define a convention that client would have to adhere to (which I find way too opinionated). The upside would be that all LC projects would follow the same structure, but it would make it hard to freely move files around. Also, I would personally avoid creating one contracts folder with all clients inside, I’d try to go for split-by-feature folder structure where each client would be encapsulated within a module that uses it (with a possibility of importing it from other modules, a practice which should generally be discouraged). Maybe that’s not a huge issue for our projects, but not leaving a backdoor for customizing the output structure could be risky in the long term. I’m curious to hear your opinions on this. Maybe I’m missing something.

[jakubfijalkowski]

but when it comes to the sole code generation supporting shared libraries doesn’t seem like that much of an issue to me

The solution that you showed adds also quite a lot complexity to the generation phase - you need to keep "class-client" relationship and, when generating the client code, prefix all the names with the scope. Otherwise, when importing the files into global scope (i.e. without as), we would need to keep names unique across different "clients" which might be quite painful in bigger projects. Nevertheless, I think that would still be better than what we have right now and the "prefixing complexity" is something we might want to have. :)

What should the output look like (structure-wise)? I guess we want to have one file per client?

I somewhat deliberately ignored that right now. I don't want to impose anything just yet on the generated code side. I don't think the clients need to look the same on both platforms, so for now you can choose whatever you want.

In the manual approach I guess we could add (possibly optional) name property which would specify the output folder:

This is the thing I want to avoid - no path overrides. Everything should be "by convention" otherwise we somewhat open pandora box of OS-dependent path handling (and self-denying configuration).

The upside would be that all LC projects would follow the same structure

That's a downside, IMO. Most of our projects feel/look similar, but not that similar. :P

I would personally avoid creating one contracts folder with all clients inside

Depends. There are projects where "split by role" works at the first level and then you split features vertically (i.e. where admin panel is completely separate to the end-user app). Others, where you handle multiple user-roles in a single app, splitting by role on the second level makes more sense (but sometimes is painful also, and it's better to have a unified API). Nevertheless, although I've given the examples that way, that's only an example - you can name the clients whatever you like even in the convention-based approach.

I am also rather against the convention method. Configuring everything manually, albeit painful, will be more future-proof.

[Albert221]

Configuring everything manually, albeit painful, will be more future-proof.

+1

[konowrockis]

I still think we need start off with more flexible way of configuring things. In FE it's pretty common to have plain .json config for something but you can as easily have the same config in .js. That gives us a lot of power as we have "sharing" out of the box. We can have callbacks. I'm not sure how we can achieve something simillar with C#. I was thinking about .csx. But I'm not sure how we would handle callbacks. I was also thinking about .lua as it's pretty easy to integrate and not complicated language overall. @jakubfijalkowski what do you think? Do you see any easy win here?

Callbacks

That's one thought that I had. If we could somehow manually interfere in the process of generating contracts that'd give us a lot of flexibility. For example if we have name clash we could just map the class name on the fly. And I mean not automatically - if we had two UserDTO we would manually map UserDTO from namespace Invoices to InvoiceUserDTO. You need to define that map manually. We have raw AST from roslyn, we already have our residual AST, we have generating code. Lot's of places to hook into. We basically need babel for our AST 😛 One simillar approach besides full AST modification that I can think of is pnpm and its readPackage feature that allows you to perform any operation on npm package before it gets installed.

Having that we could go further, we could replace matching files by regexes with custom logic - whatever you desire.

What do you think?

[konowrockis]

I'm not sure we'd only achieve flexibility, we'd achieve something. I mean if we have scripting, flexibility would come as nice addition. Now we have approach where we want to automate everything. But I feel we've kinda reached a limit where we can not automate much more. Problems we want to solve require some manual addition here and there. This is basically the same thing you described in here under Manual section. But instead of convoluted json, rules which at first look doesn't seem clear and probably still some edge cases we replace it with simple (everything_you_need_to_know_to_make_decision) => boolean. This by accident gives us basically limitless flexibility.

Also I don't think they add tremendous amount of complexity. They add some, but if we rationally define things you can hook into then it doesn't sound like that much work.

IMO scripting is the way to go.

[konowrockis]

@Albert221 Well it kinda depends. For example how would you address sharing parts of configs between separate configurations (which we need to add more if we want to split contracts). How would you address issues that @jakubfijalkowski mentioned above? How would you automatically resolve issues with name clashes? And how would you resolve all of that at once?
If aren't cases like the one you described an ideal candidate for a new key in the configuration file this was true then the configuration would be alive and we wouldn't have this conversation at all. AFAIK it pretty much haven't changed much since I've had created it probably 2+ years ago.

[jakubfijalkowski]

Disagree. Scripting in and of itself only increases complexity. You need to properly define what do you want to script to make it flexible. And properly doing that if you don't even know what you want by its very nature is tremendously hard. Example - you first need to know what the everything_you_need_to_know_to_make_decision is to make it work. It also requires quite a documentation to be usable, otherwise it will be unusable. Predefined configuration is much easier to explain and it is much harder to shoot oneself in the foot.

if we rationally define things you can hook into then it doesn't sound like that much work.

I agree - we first need to know where to hook into to properly implement this. We also need to have sensible defaults, otherwise the "scripting part" will be nightmare when starting the project. Most of our projects don't need that kind of flexibility, especially at the early stages where the thing you need more is guidance and some starting point.

how would you address sharing parts of configs between separate configurations (which we need to add more if we want to split contracts)

I don't think we need "sharing" per se. We just want sensible defaults that can be overwritten (merged). That will handle most of the cases without doing "sharing" (which we would need to define first). Sensible repetition is much better than forced DRY.

How would you automatically resolve issues with name clashes? And how would you resolve all of that at once?

Maybe the answer is - don't resolve that automatically? Developer probably knows better how to handle that. Not everything can be (or rather - should be) handled automatically without adding some strange rules that will work only sometimes.

AFAIK it pretty much haven't changed much since I've had created it probably 2+ years ago.

And that's a good thing IMO. It just means that it handled our needs. There were pain points, but not that high to force us to change that. :)

[konowrockis]

Don't agree but ok, I won't pressure scripting if there's that strong opposition.
However:

Maybe the answer is - don't resolve that automatically? Developer probably knows better how to handle that. Not everything can be (or rather - should be) handled automatically without adding some strange rules that will work only sometimes.

I mean... what? What about flat contracts in the first post? Isn't solving that automatically the whole point of this discussion? That's where the whole prefixing thing or nesting contracts came from.

I don't think we need "sharing" per se. We just want sensible defaults that can be overwritten (merged). That will handle most of the cases without doing "sharing" (which we would need to define first). Sensible repetition is much better than forced DRY.

Don't agree - if we introuduce multiple contract files and add more configuration options then defaults won't solve problems and no sharing will discourage users to split up configs.

But...

...as I understand that there are some big changes to be made on mobile I don't quite understand what do we want to achieve on FE and what we can't achieve now... Nested error codes (not sure if I wouldn't be able to workaround that now) and deprecated? Is that all? That's assuming regex is ok (I still think it is - prove me wrong if you want).

[jakubfijalkowski]

I mean... what? What about flat contracts in the first post? Isn't solving that automatically the whole point of this discussion? That's where the whole prefixing thing or nesting contracts came from.

You miss the point. I don't want to resolve name clashes in all cases (although that is one of the solutions), I just want to define areas that must have unique names. "Whole project" is just a little bit too much. Resolving name clashes won't solve the underlying solution - losing vertical slice separation in the process (hence "clients" start to emerge IMO).

Don't agree - if we introuduce multiple contract files and add more configuration options then defaults won't solve problems and no sharing will discourage users to split up configs.

I think we're talking about slightly different things. By "sharing" I mean project-level sharing, something like var sharedConfig = {}; somewhere in "shared" files and then merge(sharedConfig, {}) in each client or like anchors in YAML. I don't think we need that and that was what I've been describing. We can still have "sharing" in the sense that we have... layered configuration. Let's take simplified TypeTranslation as an example, and the "manual client definition above". We can have something like that (assuming that we don't allow mixing languages in a single configuration which, I think, is not used by anyone right now - but that might change):

{
  "language": "typescript",
  "typeTranslations": {"int": "number"},
  "rootPath": "../backend/src",
  "clients": [
    {
      "name": "a",
      "contractsRegex": ".*LeanCode.Example.Contracts(/|\\\\)(A.*|[^/\\\\]*)\\.cs$",
      "typeTranslations": { "float": "number" }, # This will get merged with the `typeTranslations` above
    },
    {
      "name": "b",
      "contractsRegex": ".*LeanCode.Example.Contracts(/|\\\\)(B.*|[^/\\\\]*)\\.cs$",
      "typeTranslations": { "float": "number" }, # This will get merged with the `typeTranslations` in the parent
    }
  ]
}

I don't think we need this configuration per client (in the sense of my previous post) since we should have the same conventions for the whole project, but if there is a need, we can have that.

...as I understand that there are some big changes to be made on mobile I don't quite understand what do we want to achieve on FE and what we can't achieve now... Nested error codes (not sure if I wouldn't be able to workaround that now) and deprecated?

You can't achieve auto-generated hierarchy with more than one level (you can still generate multiple clients but these will be flat). You can't also work on the namespace level instead of path level (which was great simplification earlier and probably will be preserved).

The main thing that I want to change in TS generator is to add explicit and mandatory hierarchy. Not "if you want to you can hack something", but explicit, easily manageable hierarchy. Something that will be enforced both on the contracts source (so - C# files), and by the generator (so that you don't end up with ExampleApi with hundreds of methods that easily). This does not mean you need to drastically change how the client looks like, but it will change how we work with it.

That's assuming regex is ok (I still think it is - prove me wrong if you want).

Don't want to. I still think it is OK (although I would probably make it work on namespace, not folder, level, but that's really just a minor change from my perspective). It's just that it can sometimes be pain in the ass.

Although we have monsters like these: (.*Contracts/Mobile.*|.*Contracts/Authorizers.*|.*Contracts/[^/]*|Apps/LeanCode.Example/Api/GenerateShareLink)\\.cs$ or LeanCode.Example\\.(Core|Clients|Shared)\\.Contracts(/|\\\\).*\\.cs(?<!Permissions\\.cs)$ (with JSON escaping included) 😛