-
-
Notifications
You must be signed in to change notification settings - Fork 4
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
Conversation
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
|
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 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? |
You're right, I think we should rework it to be more tree-shakable. 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 Perhaps I miss something; |
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 |
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. |
iam
,ec2
,elasticache
,sns
.operator
shapes from model instead of only those of theservice
shape.