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

Add official way to opt out of using Get #84

Open
liamnichols opened this issue Aug 4, 2022 · 6 comments
Open

Add official way to opt out of using Get #84

liamnichols opened this issue Aug 4, 2022 · 6 comments
Labels
enhancement New feature or request

Comments

@liamnichols
Copy link
Member

liamnichols commented Aug 4, 2022

Background

As it stands, to opt out of using Get, you must do three things:

  1. Use --module mode
  2. Update paths.imports to be an empty array
  3. Provide your own Request shim

While we believe that using CreateAPI with Get it the quickest and easiest option, we understand that not everybody is in a position to adopt Get. As a result, we need to provide a better way to disable use of the dependency.

Description

I can think of three scenarios:

  1. You want to use Get and you are happy with the default behaviour
  2. Your networking client already defines it's own Request in a module that should be imported
  3. You will build your networking client on top of the generated code and want a Request type generated for you

I did at first think that we should just generate Request if you opted not to use Get, but this will put some people who already import a different module defining Request in a situation where they can't use the generated code. It might be that like Get, the Request type is used for manually crafted requests too so defining it inside the CreateAPI generated package might not be appropriate.

Not sure how best to handle this...

@liamnichols
Copy link
Member Author

liamnichols commented Aug 4, 2022

To support all three scenarios, I was thinking that maybe we could do it via either an enum or two bools:

Default Behaviour (using Get)

paths:
  requestType: useGet
paths:
  isUsingGet: true
  isGeneratingRequestType: false

Use Different Request

paths: 
  requestType: none
paths:
  isUsingGet: false
  isGeneratingRequestType: false
  imports:
  - MyAPIClient

Generate Request

paths:
  requestType: generate
paths:
  isUsingGet: false
  isGeneratingRequestType: true

Being honest, I'm not a fan of either of these approaches. And in fact, I'm wondering if we can combine this with improvements to how imports work since we already know we need some improvements in this area.

Currently, imports is just an array of strings. But it would be powerful if imports could also describe package dependencies (i.e the source and the version requirement) so that we can generate the complete Package definition. Going one step further, we could also declare what types (or features?) package provides to infer if we need to figure it out ourself...

For example:

dependencies:
- module: CoreLocation
  in: entities # or [entities, paths]
- module: MyAPIKit
  url: https://github.com/MyName/MyAPIKit.git
  branch: main
  provides:
  - Request

This would be a powerful feature that can enable a lot:

  1. We can infer if Request needs to be generated based on the presence of a provides + if the user opted out of Get.
  2. We could also apply it to other default dependencies? Although I'm not sure if they make so much sense as they're all very small.
  3. We could apply it to any type (i.e generated types) in theory.
  4. It makes --package generation more powerful because you can define your own dependencies to be included
  5. We can relatively easily transition from the old approach since it also works with system imports/imports that aren't tied to SPM.

So back to the original examples

Default Behaviour (using Get)

# No configuration needed

Use Different Request

dependencies:
- module: MyAPIKIt
  url: https://github.com/MyName/MyAPIKit.git
  from: 1.0.0
  provides:
  - Request

Generate Request

paths:
  isGeneratingRequestType: true

The logic for importing Get would then be something like this:

var requiredDependencies: [RequiredDependency] = []
var customDependencies: [CustomDependency] = // ...
if !customDependencies.contains(where: { $0.provides.includes("Request") }) && !options.paths.isGeneratingRequest {
    requiredDependencies.append(RequiredDependency(
        module: "Get",
        importIn: [.paths],
        url: "https://github.com/kean/Get.git",
        from: "1.0.2"
    ))
}

I think this could work... I'll open a separate ticket for it 😄 Feels like a 0.2.0 thing though

@LePips
Copy link
Contributor

LePips commented Aug 11, 2022

From my comment in #85, we can create a paths.requestProvider: Get || custom option. With paths.requestProvider: Get, all we do is simply use Get as an import in Paths and add it as an import to the files.

For paths.requestProvider: custom, users have two options:

@liamnichols
Copy link
Member Author

There is still the scenario to cover where we want to generate the Request type as part of the generated output.

This is particularly useful when using create-api to generate the sources directly into an existing target where the api client lives side by side.

@LePips
Copy link
Contributor

LePips commented Aug 11, 2022

I would assume in that scenario that the Request type is a custom local type, so the user would still have to provide the Request type.

Or, we can expand the options to paths.requestProvider: Get || local || external.

  • Get, as it is now
  • local, generates a boilerplate Request type
  • external, doesn't generate anything. Request should come from some import or is defined elsewhere, like alongside an api client in the same module.

Edit: actually, since I just glanced at Get.Request, we should just have paths.requestProvider: local || external with the same reasonings. Get would have to be imported manually so that devs can ultimately choose what their networking interface is.

Or now that it's a binary option, we have the flag generateRequestType: true || false

@LePips
Copy link
Contributor

LePips commented Aug 12, 2022

Please see the bottom of the following comment first: #136 (comment)


The question regarding this specifically is: should we tightly couple CreateAPI to a specific networking package? The meaning of tightly couple is the same as in the comment.

For this specifically, my opinion is strongly no. As a generator, we don't care about the underlying networking implementation by the developer. By generalizing the Request object, we just do that: create a request and the developer passes it onto the implemented networking stack, post-processing if desired to match their stack.

As said in the attached comment, we can happily recommend Get and provide the boilerplate config options in documentation, probably when talking about how to use CreateAPI and describing the Request type. So, the generateRequestType option seems the most optimal in my opinion.

@kean
Copy link
Member

kean commented Aug 12, 2022

An enum sounds good. Maybe something like this:

apiClient

Type: Enum

Default: get

The networking library to use:

  • get – a thin wrapper on top of URLSession (? maybe name it urlsession)
  • alamofire – (?) generates Request and extensions to Alamofire that work with it
  • moya – (?) add TargetType conformance to Request
  • vapor – (?) generates code for backend-to-backend communication
  • custom – generates Request type and the user is expected to either use it directly or provide adapters for it

CreateAPI and Get are part of the same "ecosystem". It can be considered the same as using URLSession directly.

If get is used, it would be nice to also generate a few typealiases for its types so that the user won't have to think/import it. For them, it'll look like a simple URLSession-based networking stack provided by the generated package.

typealias APIClient = Get.APIClient

For more advanced use-cases (e.g. no Request type), I think CreateAPI should support templates, (Stencil?). Not even going to suggest it now though, which OK I just did :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants