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: add pluggable runtime specific config #404

Merged

Conversation

AllanZhengYP
Copy link
Contributor

@AllanZhengYP AllanZhengYP commented Oct 18, 2019

  1. Add runtime specific config runtimeConfig.ts and runtimeConfig.browser.ts. I use browser property to swap the config according to build target. This nice thing of it is that when bundler cannot correctly bundle the client, users can supply the const from the runtimeConfig to the client the client will be target at right runtime. So no rollup pre-bundling is needed.

  2. Fix the exerializer to take endpoint from resolved client config

  3. Rename HttpEndpoint to Endpoint in @aws-sdk/types because it is suppose to be non http specific

/cc @srchase Do merge it with your future PR and feel free to close it. Also please rename AWSClientRuntimeConfiguration to RDSDataRuntimeConfiguration

Note: I managed to make it work in browser although RDS-Data doesn't support CORS. So this PR contains some small bug fixing. My local webpack production bundle with 1 command takes 61 KB whereas the released version @aws-sdk/client-rds-data-browser takes 76KB. I assume that's because of the optimization from ESModule and side-effect config

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

This comment has been minimized.

@AllanZhengYP AllanZhengYP added the smithy-codegen Changes regarding supporting smithy model. Will be merged to smithy-codegen branch label Oct 18, 2019
@trivikr trivikr self-assigned this Oct 18, 2019
packages/invalid-dependency/src/index.ts Show resolved Hide resolved
packages/types/src/serializer.ts Outdated Show resolved Hide resolved
packages/url-parser-universal/src/index.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,30 @@
import { invalidFunction } from '@aws-sdk/invalid-dependency';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prettier wasn't run on this file.
Please cut a separate issue for tracking if git hooks aren't working

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you know it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Single quotes 😉

If this is an issue with git-hooks, it can be tracked in a separate issue

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out my local workspace's formatter did set to vscode default ones. So I added a vscode.settings: "editor.defaultFormatter": "esbenp.prettier-vscode"

@codecov-io
Copy link

codecov-io commented Oct 21, 2019

Codecov Report

Merging #404 into smithy-codegen will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@                Coverage Diff                 @@
##           smithy-codegen     #404      +/-   ##
==================================================
+ Coverage           96.72%   96.74%   +0.02%     
==================================================
  Files                  69       70       +1     
  Lines                1161     1169       +8     
  Branches              214      214              
==================================================
+ Hits                 1123     1131       +8     
  Misses                 38       38
Impacted Files Coverage Δ
packages/url-parser-browser/src/index.ts 100% <100%> (ø) ⬆️
packages/url-parser-node/src/index.ts 72.72% <100%> (ø) ⬆️
packages/stream-collector-browser/src/index.ts 100% <0%> (ø)

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...a7b4ef8. Read the comment docs.

@aws-sdk-js-automation

This comment has been minimized.

@aws-sdk-js-automation

This comment has been minimized.

* export runtimeConfig.runtime.ts to manually set the client to be compatible with specific runtime
* get rid of rollup, instead using browser property to swap runtime config
* add endpoint to the serializer utilities and insert it when building a request
@aws-sdk-js-automation
Copy link

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

trivikr added a commit to trivikr/aws-sdk-js-v3 that referenced this pull request Oct 21, 2019
@AllanZhengYP AllanZhengYP merged commit 3aaa7c2 into aws:smithy-codegen Oct 22, 2019
@lock
Copy link

lock bot commented Oct 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 Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
smithy-codegen Changes regarding supporting smithy model. Will be merged to smithy-codegen branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants