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

feat: improve client generation #48

Closed
wants to merge 2 commits into from
Closed

feat: improve client generation #48

wants to merge 2 commits into from

Conversation

godu
Copy link
Contributor

@godu godu commented Apr 18, 2024

  • Regenerates clients : iam, ec2, elasticache, sns.
  • Improves codegen with some ideas from Generating all aws clients #46
    • Exports exceptions with the same name as the original package
     import type {
       ...
     -   UnrecognizedPublicKeyEncodingException,
     +   UnrecognizedPublicKeyEncodingException as SdkUnrecognizedPublicKeyEncodingException,
       ...
     } from "@aws-sdk/client-iam";
     - export type `ConcurrentModificationError` =
     -   TaggedException<ConcurrentModificationException>;
     + export type ConcurrentModificationException =
     +   TaggedException<SdkConcurrentModificationException>;
    • Uses all operator shapes from model instead of only those of the service shape.
    • Setup default values of client generation.

@joepjoosten
Copy link

I've already experienced an error when the aws-sdk version that is used to generate the client was higher then the the aws-sdk used in a project. Maybe it's good to pin the version of the aws-sdk, or at least be minimum the version used to generate?

e.g. package.json of client-cloudwatch-events

    "@aws-sdk/client-cloudwatch-events": "^3.556.0",
    "@aws-sdk/types": "^3.556.0"

@godu godu changed the title feat: adds some clients feat: improve client generation Apr 24, 2024
@joepjoosten
Copy link

joepjoosten commented Apr 30, 2024

Hi, i'm using my own generated aws clients based on the code from the effect-aws repo (my pullrequest). And i've noticed that the tree shaken esm build is still rather large. This is because the generated service contains a:

const commands = {
  all commands...
}

Which can't be tree shaken. So it also will include all imports of the commands...
This is really large, even if you only use one command in your function. I've been tinkering with the implementation, and i think i found a solution, which is maybe also more like the methods and functions in the effect library itself:

This is an example of the implementation for the SSM client:

export const updateServiceSetting: (
  args: UpdateServiceSettingCommandInput,
  options?: __HttpHandlerOptions,
) => Effect.Effect<
  UpdateServiceSettingResult,
  | SdkError
  | InternalServerError
  | ServiceSettingNotFound
  | TooManyUpdates,
  SSMClientInstance
> = typedErrors(UpdateServiceSettingCommand);

function typedErrors<R, E, T extends new (...args: any) => any>(command: T) {
  return (args: ConstructorParameters<T>[0], options?: __HttpHandlerOptions) => Effect.gen(function* (_) {
    const client = yield* _(SSMClientInstance);
    return yield* _(Effect.tryPromise({
        try: () => client.send(new command(args), options),
        catch: (e) => {
          if (e instanceof SdkSSMServiceException) {
            const ServiceException = Data.tagged<
              TaggedException<SdkSSMServiceException>
            >(e.name);
  
            return ServiceException({
              ...e,
              message: e.message,
              stack: e.stack,
            });
          }
          if (e instanceof Error) {
            return SdkError({
              ...e,
              name: "SdkError",
              message: e.message,
              stack: e.stack,
            });
          }
          throw e;
        },
      }) as Effect.Effect<R, E>)
  });
}

What is your opinion?

@godu
Copy link
Contributor Author

godu commented May 2, 2024

You're right, I think we should rework it to be more tree-shakable.
I don't like your proposal because you embed the @AWS-SDK binding in the effect logic. I think, like in hexagone architecture's ports do, we should scope this part in the service.

I tried something here (the typing could be improved). I create a Service for each Command.

It'll be easier to mock it.

    const listRegionsServiceMock = {
      listRegions: vi.fn().mockImplementation(() => Effect.succeed({})),
    };

    const result = await pipe(
      program,
      Effect.provide(Layer.succeed(ListRegionsService, listRegionsServiceMock)),
      Effect.runPromiseExit,
    );

    expect(result).toEqual(Exit.succeed({}));
    expect(listRegionsServiceMock.listRegions).toHaveBeenNthCalledWith(1, {});

But the metafile still show me the all @AWS-SDK's commands and the @effect-aws's unused services.

esbuild --minify --format=esm --target=es2022 --bundle --platform=node test/input.ts --metafile=dist/meta.json --outfile=dist/output.js --tsconfig=tsconfig.esm.json

meta.json

Perhaps I miss something;

@joepjoosten
Copy link

I've fixed this by creating a separate file per command. This will fix the issue with unused imports. The only duplication left is something that needs to be solved in esbuild: evanw/esbuild#475

@joepjoosten
Copy link

joepjoosten commented May 4, 2024

I don't like the approach to have a service per command. Then you need to create a layer for all commands used in the underlying effects. This will reduce re-usability, and creating a default layer with all services is not tree shake able again...

You are right that it's much nicer when creating mocks for a service. But i think using something like aws-sdk-client-mock can help creating mocks. I'm currently trying this out, how this would look like.

@godu godu closed this Sep 17, 2024
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.

2 participants