feat: standardize plugin naming, add getMiddleware fn [WIP] #402

srchase · 2019-10-17T20:21:08Z

Standardizes naming of plugins. Use resolveConfig and getMiddleware.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

aws-sdk-js-automation · 2019-10-17T20:24:19Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-sdk-js-automation · 2019-10-17T20:44:13Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-sdk-js-automation · 2019-10-17T21:19:39Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

codecov-io · 2019-10-17T21:26:55Z

Codecov Report

Merging #402 into smithy-codegen will increase coverage by 0.25%.
The diff coverage is 73.33%.

@@                Coverage Diff                 @@
##           smithy-codegen     #402      +/-   ##
==================================================
+ Coverage           96.72%   96.97%   +0.25%     
==================================================
  Files                  69       69              
  Lines                1161     1158       -3     
  Branches              214      214              
==================================================
  Hits                 1123     1123              
+ Misses                 38       35       -3

Impacted Files	Coverage Δ
packages/middleware-retry/src/constants.ts	`100% <ø> (ø)`
packages/middleware-retry/src/defaultStrategy.ts	`100% <ø> (ø)`
packages/middleware-retry/src/delayDecider.ts	`100% <ø> (ø)`
packages/middleware-retry/src/retryDecider.ts	`100% <ø> (ø)`
packages/middleware-signing/src/middleware.ts	`100% <100%> (ø)`
packages/middleware-retry/src/retryMiddleware.ts	`100% <100%> (ø)`
packages/middleware-retry/src/configurations.ts	`66.66% <50%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1bb6e37...a6939dd. Read the comment docs.

AllanZhengYP · 2019-10-17T22:16:53Z

Can you keep the CHANGELOG untouched? So we have a clue on what they are renamed from.

AllanZhengYP · 2019-10-17T22:28:21Z

clients/node/client-rds-data-node/RdsDataServiceClient.ts

+    let _config_5 = UserAgentConfig.resolve(_config_4);
+    super(_config_5);
+    this.config = _config_5;
+    super.use(AwsAuthConfig.getMiddleware(this.config));


I'm not a fan of getMiddleware(), because it doesn't give you a middleware. It in-fact mutates the client's middleware stack. Also the all namespace XXXConfig doesn't make sense because they are not really Config. I still prefer the naming convention of client config component's named XXXConfig. If they have middleware to insert, then it should export a XXXPlugin. If destroy method is required, then it should export a method called destroyXXX()

I removed the Config suffix and switched to resolveConfig and setMiddleware to make this more clear. Using the name for a plugin, along with same function for resolveConfig, destroy, etc will be easier to codegen.

I'm not a fan of getMiddleware(), because it doesn't give you a middleware. It in-fact mutates the client's middleware stack.

Is this true? Or is this going to be true after some future changes? This code looks like it's getting a list of middleware to inject into this.use by calling getMiddleware.

I still prefer the naming convention of client config component's named XXXConfig. If they have middleware to insert, then it should export a XXXPlugin. If destroy method is required, then it should export a method called destroyXXX()

Can you give concrete examples?

@mtdowling

This code looks like it's getting a list of middleware to inject into this.use by calling getMiddleware

I updated the use interface recently which takes in a function mutating the middleware stack, see here, and here.
A runtime plugin is not just a list of middleware, it may also include mutating existing middleware stack and even delete some middleware if exist. This interface allows more extensibility than only allowing injecting new middleware.
Using setMiddleware here is better than getMiddleware. I will propose getPlugin as it better describe the function.

Can you give concrete examples?

AwsAuth customization has input config interface, so ClientConfiguration contains AwsAuthInput, and AwsAuth.Resolved. The resolveConfig should also be called.

AwsAuth customization has middleware stack customization(runtime plugin), so client should use the plugin. But here I suggest renaming it to super.use(AwsAuth.getPlugin(this.config))).

AwsAuth doesn't have a destroy so take ProtocolConfig as an example, because it contains handler. The client destroy() will look like:

destroy(): void { ClientProtocol.destroy(this.config); }

The destroy function will call the destroy function on this.config.protocol and hence destroy the handler. The this.config is supplied to function because destroyable resources are in the client's resolved config

I updated the use interface recently which takes in a function mutating the middleware stack, see here, and here.

👍

Using setMiddleware here is better than getMiddleware

👍

I will propose getPlugin as it better describe the function.

I think setMiddleware of updateMiddleware makes more sense. It says exactly what the function does. getPlugin doesn't convey anything really, and it makes the term "plugin" more ambiguous in the SDK. We need to be really deliberate with how we describe things across codegen and runtime packages.

AwsAuth customization has input config interface, so ClientConfiguration contains AwsAuthInput, and AwsAuth.Resolved. The resolveConfig should also be called.

Why the difference between AwsAuthInput and AwsAuth.Resolved? Seems like a strange pattern. If there's some technical limitation in IDEs, then we should standardize on AwsAuthInput and AwsAuthResolved instead to make this less surprising.

If we do that, then we should get rid of the namespacing altogether and do something like:

AwsAuthConfigInput interface

AwsAuthConfigResolved interface

resolveAwsAuthConfig() function

updateAwsAuthMiddleware() function

My biggest gripe with the above code is that it's leaning way too much on convention and spreading the responsibility out across so many different disconnected things.

Alternatively we could have a different convention for bridging codegen and runtime code references. For example we could provide a set of interfaces that have generic parameters for the input type, resolved type, and methods for resolveConfig and updateMiddleware.

Why the difference between AwsAuthInput and AwsAuth.Resolved? Seems like a strange pattern.

Yes. It is an IDE limitation. It makes sense to get rid of namespace.

I'm still a little concern about the updateAwsAuthMiddleware. It sounds like it takes a parameter that is a middleware to be updated. In fact the Injectable interface is the "Plugin" interface we expose(I think we should rename it to Pluggable). The updateAwsAuthMiddleware() is actually a getter of a Pluggable, this is why I suggest getPlugin.

we could provide a set of interfaces that have generic parameters for the input type, resolved type, and methods for resolveConfig and updateMiddleware.

I prefer using convention over interface because by using interface, these config and middleware plugins will be attached to a single object. If you import one of them in client, you import all of them. But for customizations that requires client config and middleware in the commands(there are many), we don't want to import middleware plugin in the client. This will increase the size of the client.

Basically the config and middleware plugin are better to be decoupled.

aws-sdk-js-automation · 2019-10-18T15:16:25Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-sdk-js-automation · 2019-10-18T15:24:32Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-sdk-js-automation · 2019-10-18T15:53:07Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

packages/middleware-content-length/src/index.ts

aws-sdk-js-automation · 2019-10-21T15:52:04Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

AllanZhengYP · 2019-10-28T02:42:18Z

This PR has been replaced to #422

lock · 2019-11-04T03:24:42Z

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.

feat: standardize Config naming, add getMiddleware fn

03be759

srchase added the smithy-codegen Changes regarding supporting smithy model. Will be merged to smithy-codegen branch label Oct 17, 2019

Chase Coalwell added 2 commits October 17, 2019 13:34

feat: update client naming

b5fbb49

fix: add back protocol config

032274d

fix: remove structValue on visitor

d48d48f

srchase requested a review from AllanZhengYP October 17, 2019 21:56

AllanZhengYP reviewed Oct 17, 2019

View reviewed changes

Chase Coalwell added 3 commits October 17, 2019 15:40

fix: remove Config naming

f183659

feat: add ContentLength

b5f6d10

feat: change to resolveConfig and setMiddleware

34ca343

fix: resolveConfig on config-resolver

d7d11ff

fix: revert changelog changes

d7c6dad

srchase changed the title ~~feat: standardize Config naming, add getMiddleware fn~~ feat: standardize Config naming, add setMiddleware fn Oct 18, 2019

srchase changed the title ~~feat: standardize Config naming, add setMiddleware fn~~ feat: standardize plugin naming, add getMiddleware fn Oct 18, 2019

trivikr self-assigned this Oct 18, 2019

AllanZhengYP reviewed Oct 19, 2019

View reviewed changes

packages/middleware-content-length/src/index.ts Outdated Show resolved Hide resolved

fix: clean up commented

a6939dd

srchase changed the title ~~feat: standardize plugin naming, add getMiddleware fn~~ feat: standardize plugin naming, add getMiddleware fn [WIP] Oct 24, 2019

AllanZhengYP mentioned this pull request Oct 28, 2019

feat: standardize plugins #422

Merged

AllanZhengYP closed this Oct 28, 2019

lock bot locked as resolved and limited conversation to collaborators Nov 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: standardize plugin naming, add getMiddleware fn [WIP] #402

feat: standardize plugin naming, add getMiddleware fn [WIP] #402

srchase commented Oct 17, 2019 •

edited

Loading

aws-sdk-js-automation commented Oct 17, 2019

aws-sdk-js-automation commented Oct 17, 2019

aws-sdk-js-automation commented Oct 17, 2019

codecov-io commented Oct 17, 2019 •

edited

Loading

AllanZhengYP commented Oct 17, 2019

AllanZhengYP Oct 17, 2019 •

edited

Loading

srchase Oct 18, 2019 •

edited

Loading

mtdowling Oct 18, 2019

AllanZhengYP Oct 19, 2019 •

edited

Loading

mtdowling Oct 19, 2019

AllanZhengYP Oct 21, 2019 •

edited

Loading

aws-sdk-js-automation commented Oct 18, 2019

aws-sdk-js-automation commented Oct 18, 2019

aws-sdk-js-automation commented Oct 18, 2019

aws-sdk-js-automation commented Oct 21, 2019

AllanZhengYP commented Oct 28, 2019

lock bot commented Nov 4, 2019

feat: standardize plugin naming, add getMiddleware fn [WIP] #402

feat: standardize plugin naming, add getMiddleware fn [WIP] #402

Conversation

srchase commented Oct 17, 2019 • edited Loading

aws-sdk-js-automation commented Oct 17, 2019

AWS CodeBuild CI Report

aws-sdk-js-automation commented Oct 17, 2019

AWS CodeBuild CI Report

aws-sdk-js-automation commented Oct 17, 2019

AWS CodeBuild CI Report

codecov-io commented Oct 17, 2019 • edited Loading

Codecov Report

AllanZhengYP commented Oct 17, 2019

AllanZhengYP Oct 17, 2019 • edited Loading

Choose a reason for hiding this comment

srchase Oct 18, 2019 • edited Loading

Choose a reason for hiding this comment

mtdowling Oct 18, 2019

Choose a reason for hiding this comment

AllanZhengYP Oct 19, 2019 • edited Loading

Choose a reason for hiding this comment

mtdowling Oct 19, 2019

Choose a reason for hiding this comment

AllanZhengYP Oct 21, 2019 • edited Loading

Choose a reason for hiding this comment

aws-sdk-js-automation commented Oct 18, 2019

AWS CodeBuild CI Report

aws-sdk-js-automation commented Oct 18, 2019

AWS CodeBuild CI Report

aws-sdk-js-automation commented Oct 18, 2019

AWS CodeBuild CI Report

aws-sdk-js-automation commented Oct 21, 2019

AWS CodeBuild CI Report

AllanZhengYP commented Oct 28, 2019

lock bot commented Nov 4, 2019

srchase commented Oct 17, 2019 •

edited

Loading

codecov-io commented Oct 17, 2019 •

edited

Loading

AllanZhengYP Oct 17, 2019 •

edited

Loading

srchase Oct 18, 2019 •

edited

Loading

AllanZhengYP Oct 19, 2019 •

edited

Loading

AllanZhengYP Oct 21, 2019 •

edited

Loading