Suggestion: Directly exporting service actions

[simonbuchan]

At the moment the packages export a similar structure to v2, a service class with methods for each action. In theory, you can do better at tree-shaking and possibly make test mocking easier with a direct function export for each action taking a configuration or context as a parameter, e.g., instead of:

import { Foo } from "@aws-sdk/client-foo-node"

let foo: Foo;
export function init(config) {
    const foo = new Foo(config);
}

export async function action(params) {
    await foo.bar(params);
}

doing something like:

import * as Foo from "@aws-sdk/client-foo-node"

let client: Foo.FooClient;
export function init(config) {
    client = new Foo.FooClient(config);
}
export async function action(params) {
    await Foo.bar(context, params);
}

I'm not sure exactly how much impact this could have, but it looks like it could let an optimiser remove all of the unused command/* and model/* classes. Since the usages I've seen in our company have been very commonly a small handful of actions spread over multiple services, it seems like this could be a pretty good win.

Obviously, this means testing can't just pass a mock client object to the tested methods, but it's pretty standard usage to init the client in the module of the unit to be tested (like above), which means your test mocking would already need to handle mocking the client creation and the method already, so this would be about the same.

Using a fake mocking library for illustration, this would require a particularly nosy test to change from:

// ...
class MockFoo {
  constructor(public config) {}
  bar = mock.fn(1);
}

test("action1 calls bar on arg client", async () => {
  const { action1 } = await import("./testee");
  const client = new MockFoo();
  await action1(client, mock.object(1));
  assert(mock.fn.single.calls.single.args(client, mock.object(1)));
});

test("action2 calls bar on module client", async () => {
  mock.module("@aws-sdk/client-foo-node", {
    Foo: MockFoo,
  });
  const { init, action2 } = await import("./testee");
  init(mock.object(1));
  await action2(mock.object(2));
  assert(mock.fn.single.calls.single.args(
    mock.instance(MockFoo, { config: mock.object(1) }),
    mock.object(2)));
});

to:

// ...
class MockFoo {
  constructor(public config) {}
}
test("action1 calls bar on arg client", async () => {
  mock.module("@aws-sdk/client-foo-node", {
    bar: mock.fn(1),
  });
  const { action1 } = await import("./testee");
  const client = new MockFoo();
  await action1(client, mock.object(1));
  assert(mock.fn(1).calls.single.args(undefined, client, mock.object(1)));
});

test("action calls bar on module client", async () => {
  mock.module("@aws-sdk/client-foo-node", {
    Foo: MockFoo,
    bar: mock.fn(1),
  });
  const { init, action2 } = await import("./testee");
  init(mock.object(1));
  await action2(mock.object(2));
  assert(mock.fn(1).calls.single.args(
    undefined,
    mock.instance(MockFoo, { config: mock.object(1) }),
    mock.object(2)));
});

That is, it allows decoupling mocking the client from mocking the action at the cost of now requiring module mocking for users that were passing the client as an arg before. In my experience, this would be a good trade-off, but it would be good to get more feedback.

A compromise would be to document the effect of the action function on the client (currently, send()?), so the test could only mock the client and assert the expected calls, but that seems like it could be restrictive?

This might also allow creating and sharing the middleware directly between multiple services, if they could be compatible: e.g. multiple JSON-based services in the same region, enforced by the typing system, but I'm not sure if that's reasonable?

[chrisradek]

@simonbuchan
Being able to import just the commands you want to use was one of the design goals for the V3 SDK.

Each service exports a service client, as well as command classes that represent each operation. Due to the way the transpilation is being handled today, to take full advantage of the tree-shaking capabilities of tools like webpack, you would want to import commands from their respective submodule.

For example, using a Kinesis node.js service client, you could write code like this today:

import {PutRecordCommand} from '@aws-sdk/client-kinesis-node/commands/PutRecordCommand';
import {KinesisClient} from '@aws-sdk/client-kinesis-node/KinesisClient';

export async function handler() {

    const client = new KinesisClient({
        region: 'us-west-2'
    });

    try {
        const command = new PutRecordCommand({
            StreamName: 'simple-example',
            PartitionKey: `partition-${Date.now()}`,
            Data: JSON.stringify({
                payload: 'sample!'
            })
        });

        const result = await client.send(command);

        console.log(`Put data to shard ${result.ShardId} with sequence number ${result.SequenceNumber}`);
    } catch (err) {
        console.error(err);
        throw err;
    }
}

This results in a substantially smaller bundle than if you just imported the Kinesis class and called the methods off of that.

I would need to think what this means as far as mocking goes, but you could update the middlewareStack of your command so that it returns the results you want to test, before calling client.send(command).

I'm sure the team would love to hear your feedback on the above, and any suggestions on how to improve on it, or where it's doing well!

[simonbuchan]

Ah, so that's what the Foo / FooClient split is for!

I think the above design is pretty nice, but having the easy / default path pull in everything is not so great. As an example, rxjs somewhat recently had a big restructure after a bunch of attempts at reducing their bundle size. They had a default path that looked something like:

import { Observable } from "rxjs"; // pulls in everything without specific steps
Observable.fromEvent(target, "event").switchMap(...); // can directly use nearly everything from Observable

and after a few iterations were suggesting a very similar approach as above for the users that wanted a better bundle size:

import { fromEvent } from "rjxs/observables/fromEvent";
import { switchMap } from "rxjs/operators/switchMap";

fromEvent(target, "event").pipe(switchMap(...)); // weird syntax is to simulate |> proposal, for chaining.

but this, while it works, has some bad developer ergonomics - it's far to easy for your IDE to pull in "rxjs" just by typing the wrong thing somewhere, to the point that Jetbrain's WebStorm added a "never import from ..." feature, with a default of "rjxs"!
Further, it was often viewed as an "advanced" approach, so most documentation and examples were still using the old design. This often ended up with third-parties that used it pulling in the whole thing, since their author hadn't considered that they might be used in a bundled context.

They finally settled on just exporting the free functions as the only public way to use them after webpack added support for "sideEffects": false, which made the only option:

// using namespace import to illustrate that sideEffects removes the need for lots of imports
import * as rx from "rxjs"; // all types, factories, etc...
import * as ops from "rxjs/operators"; // operators in a separate package

rx.fromEvent(target, "event").pipe(ops.switchMap(...));

The current design also has the fact that Foo extends FooClient, which makes it a little easy to miss that the "wrong" one is being used.

TL;DR: this is a great option, but I think you should at least consider making it the only one, or at least the default one, for ecosystem reasons.

[chrisradek]

@simonbuchan
Thanks for sharing, I wasn't aware of the sideEffects: false property for webpack. Do you know if other bundlers have a similar configuration option?

Just to summarize then, would you expect to see something like this then to be the default scenario?

// only exports the base client without service-specific methods attached
import {Client} from '@aws-sdk/client-kinesis-node';
import {PutRecordCommand} from '@aws-sdk/client-kinesis-node/commands';

Then if someone wanted the V2-equivalent client, they would import that from a different sub-module?
import {Client} from '@aws-sdk/client-kinesis-node/mega-client';

I haven't had a chance to play with the sideEffects feature, but I'm also wondering if that would result in smaller bundle sizes with the SDK in its current state if you import the specific commands and the FooClient from the root package (@aws-sdk/client-kinesis-node).

[simonbuchan]

Strictly you don't need to have .../commands either, I think rxjs did that just to separate some odd cases like combineLatest that have both factory and operator forms.

Looks like rollup doesn't support sideEffects yet, but do have the nuclear option treeshake.pureExternalModules, and Fuzebox has it's own wacky production bundler, that seems like it is claiming it's not as much of a problem? But in any case, while sideEffects: false might fix tree shaking for users who import your index, it's not really the root issue, which is that you don't publish ESM, which means that every module is a side-effect to the bundler!

To fix this, you have to build twice: with typescript module set to commonjs and to es2015 (or esnext for import()), and pointing to the es build index with module; the first is for raw node usage, the latter for bundlers, node --experimental-module, and direct browser modules support (presumably with HTTP2 server push for performance!)

From my understanding the most popular option to implement this is to pick a specific set of importable paths to support, commonly just the package root, in rxjs's case also rxjs/operators, that will each have a package.json with a main pointing at the commonjs entrypoint and module pointing at the esm entrypoint, e.g. "main": "lib/index.js", "module": "es/index.js". This is easy enough to do with tsc, just add --module es2015 --outDir es (but note that changing --module means the default moduleResolution is no longer node)

This won't work with node --experimental-modules, though, where the file must have the extension .mjs to be parsed as ESM. Since this isn't supported by tsc yet, you will need to do extra work to support it. If you do, ideally your package contains foo.js, foo.mjs, foo.d.ts for every import path foo, and be careful to not include the .js extension in your "main" so node picks up the .mjs if it's enabled. In this case, you don't need a module field, as current bundlers all look up .mjs first.

This is digressing even further, but if you're messing with package contents for size concerns, you might also want to consider building less downleveled code: it can have a noticable impact on code size, performance and ease of debugging (particularly with native async functions in es2017)

You can simply do this by default (es2018 is pretty safe in current browsers and versions of node, but lambda's node 8 basically has es2017), with the cost that some of your users still supporting e.g. IE might need to re-transpile your code; as a third (or more) target, and reference them from a custom package.json field, which Webpack supports via resolve.mainFields, for example you could set this to ["es2018", "module", "main"], and it would try to use the package.json field es2018 before the default module or main; or simply publish multiple packages.

Sorry for the dump, the ecosystem has gotten pretty complex around this area!

[chrisradek]

Thanks for all the info! Yes, there are definitely some changes that can be made with regards to what version of EcmaScript the SDK gets transpiled down to, and also supporting multiple import strategies. Looking forward to the day ES2015 imports are supported by default in all runtime environments!

[github-actions]

Greetings! We’re closing this issue because it has been open a long time and hasn’t been updated in a while and may not be getting the attention it deserves. We encourage you to check if this is still an issue in the latest release and if you find that this is still a problem, please feel free to comment or open a new issue.

[trivikr]

Reopening as needs-discussion label is added

[simonbuchan]

The title is a bit outdated given you do export actions in a different way that I was expecting, and the decision on #364 implies that nothing's going to be changing around this. Not sure exactly what @AllanFly120 added the label for, but it seems it's about the bundling/downleveling situation, which I suspect is quite different now. I'm happy with this being closed, assuming there's nothing I missed?

[alexforsyth]

Closing as discussion has been addressed

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.