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

Stop exporting V2-style service client #364

Closed
AllanZhengYP opened this issue Sep 16, 2019 · 13 comments
Closed

Stop exporting V2-style service client #364

AllanZhengYP opened this issue Sep 16, 2019 · 13 comments
Labels
feature-request New feature or enhancement. May require GitHub community feedback. needs-discussion

Comments

@AllanZhengYP
Copy link
Contributor

Is your feature request related to a problem? Please describe.

Now with V3 you can import a service client like:

const {DynamoDB} = require('@aws-sdk/client-dynamodb-browser');
const client = new DynamoDB({region, credential});
client.listTables({}, callback);

This makes switching from V2 SDK as smooth as possible. But it introduces a significant overhead code-size-wise. Because the class with all the available operations can be huge.
Whereas this way you can acheive significantly smaller bundle size:

const {DynamoDBClient, ListTablesCommand} = require('@aws-sdk/client-dynamodb-browser');
const client = new DynamoDB({region, credential});
const listTables = new ListTablesCommand({});
client.send(listTables).then(callback);

This makes code size smaller. It also works better on serverless services(i.e. AWS Lambda). But the V2 bridging interface make users confuse like #261

Describe the solution you'd like
Remove the V2-like client class like: https://github.com/aws/aws-sdk-js-v3/blob/master/clients/browser/client-dynamodb-browser/DynamoDB.ts in every service package

@AllanZhengYP AllanZhengYP added feature-request New feature or enhancement. May require GitHub community feedback. needs-discussion labels Sep 16, 2019
@trivikr
Copy link
Member

trivikr commented Sep 17, 2019

Removing v2-style service client won't reduce bundle size for importing clients and commands separately. Although v3-style helps reduce bundle size, it requires more code to be written and more imports to be managed. Some developers might prefer simplicity over performance.

@AllanZhengYP
Copy link
Contributor Author

@trivikr Yea removing the V2 stype client will reduce the confusion. Some user gave us some very helpful feedback here also: #213 (comment)

@Amplifiyer
Copy link
Contributor

I agree that the V3 style of imports should be default, but I don't think we should take away the V2 style just yet. It's substantially more work for customers to move from V2 to V3 using command interface and the flexibility provided in V3 is great to allow customers do it at their own pace (or in a timely fashion you can deprecate V2 style clients).

@trivikr
Copy link
Member

trivikr commented Nov 21, 2019

It's substantially more work for customers to move from V2 to V3 using command interface

Example diff for switching to v2 style:

-import AWS from "aws-sdk";
+import { DynamoDB } from "@aws-sdk/client-dynamodb";
-const client = new AWS.DynamoDB();
+const client = new DynamoDB();
/* Code to populate params inside a function */
-const result = await client.query(params).promise();
+const result = await client.query(params);

Example diff for switching to command style:

-import AWS from "aws-sdk";
+import { DynamoDBClient, QueryCommand } from "@aws-sdk/client-dynamodb";
-const client = new AWS.DynamoDB();
+const client = new DynamoDBClient();
/* Code to populate params inside a function */
-const result = await client.query(params).promise();
+const result = await client.send(new QueryCommand(params));

Of course the more commands customers use, they'll have to import them in command style. Assuming that they've written code such that only specific commands are used per file, it shouldn't be an issue.
If their code is calling multiple commands in single file (say 10+), then the code needs to be refactored.

@Amplifiyer Are there other concerns over the work required in migration?

@Amplifiyer
Copy link
Contributor

It's much straightforward for customers who are using modular client imports from V2 SDK such as import * as DynamoDB from 'aws-sdk/clients/dynamodb'. All they need to do is update the import and everything works as is. While in command interface, they have to update the code everywhere a service call is happening (not just the client generation).

@trivikr
Copy link
Member

trivikr commented Nov 21, 2019

All they need to do is update the import and everything works as is

They also need to update commands to remove .promise() call:

-import * as DynamoDB from "aws-sdk/clients/dynamodb";
+import * as db from "@aws-sdk/client-dynamodb";
 
-const client = new DynamoDB();
+const client = new db.DynamoDB();
 /* Code to populate params inside a function */
-const result = await client.query(params).promise();
+const result = await client.query(params);

In modular style, they'll have to create Command object in addition to remove .promise() as follows:

-import * as DynamoDB from "aws-sdk/clients/dynamodb";
+import * as db from "@aws-sdk/client-dynamodb";
 
-const client = new DynamoDB();
+const client = new db.DynamoDBClient();
 /* Code to populate params inside a function */
-const result = await client.query(params).promise();
+const result = await client.send(new db.QueryCommand(params));

@Amplifiyer
Copy link
Contributor

I see, maybe customers using callbacks in V2 wouldn't have to change anything?

@ajredniwja
Copy link
Member

I agree that removing v2 style clients will result in reducing the confusion.

@trivikr
Copy link
Member

trivikr commented Nov 21, 2019

If customers use .then, they still have to remove promise as follows:

-import * as DynamoDB from "@aws-sdk/client-dynamodb";
+import * as db from "@aws-sdk/client-dynamodb";
 
-const client = new DynamoDB();
+const client = new db.DynamoDB();
 /* Code to populate params inside a function */
-const result = client.query(params).promise().then((err, data) => {
+const result = client.query(params).then((err, data) => {
   // Do stuff with err or data
});

For modular style, they'll have to add the command in addition to removing .promise() as follows:

-import * as DynamoDB from "@aws-sdk/client-dynamodb";
+import * as db from "@aws-sdk/client-dynamodb";
 
-const client = new DynamoDB();
+const client = new db.DynamoDBClient();
 /* Code to populate params inside a function */
-const result = client.query(params).promise().then((err, data) => {
+const result = client.send(new db.QueryCommand(params)).then((err, data) => {
   // Do stuff with err or data
 });

@trivikr trivikr pinned this issue Nov 21, 2019
@trivikr
Copy link
Member

trivikr commented Nov 21, 2019

maybe customers using callbacks in V2 wouldn't have to change anything?

That's right, the callbacks will work in idiomatic way.

v2-style:

-import * as DynamoDB from "@aws-sdk/client-dynamodb";
+import * as db from "@aws-sdk/client-dynamodb";
  
-const client = new DynamoDB();
+const client = new db.DynamoDB();
  /* Code to populate params inside a function */
 const result = client.query(params, (err, data) => {
    // Do stuff with err or data
 });

command style:

-import * as DynamoDB from "@aws-sdk/client-dynamodb";
+import * as db from "@aws-sdk/client-dynamodb";
  
-const client = new DynamoDB();
+const client = new db.DynamoDB();
  /* Code to populate params inside a function */
-const result = client.query(params, (err, data) => {
+const result = client.send(new QueryCommand(params)).then((err, data) => {
    // Do stuff with err or data
 });

Given that JS SDK v3 encourages using await everywhere (README/API reference/Developer Guide), I think users will stick to callbacks only if it's very difficult to find time/resources for additional work required to switch to await.

In the case where callbacks are still used, I think customers will mostly continue to use v2, and write their new projects/features in v3.

@trivikr
Copy link
Member

trivikr commented Nov 22, 2019

Responses from Gitter channel:
Screenshot from 2019-11-22 08-54-13

@trivikr
Copy link
Member

trivikr commented Nov 22, 2019

Resolving as we decided to keep v2-style (legacy) clients in v3 for ease of migration

@lock
Copy link

lock bot commented Nov 29, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Nov 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request New feature or enhancement. May require GitHub community feedback. needs-discussion
Projects
None yet
Development

No branches or pull requests

4 participants