-
Notifications
You must be signed in to change notification settings - Fork 96
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
Add sSDK runtime support for NodeJS. #703
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not much familiar with NodeJS's http module but looks good mostly. If your testing reveals some edges they can be fixed.
@@ -0,0 +1,53 @@ | |||
/* | |||
* Copyright 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need the year. See #692 (though I'm not sure the checkstyle is being applied to ts files)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checkstyle is only checking java files: https://github.com/awslabs/smithy-typescript/blob/main/config/checkstyle/checkstyle.xml#L26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll adapt the files I'm creating or modifying to use the short-form license form:
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const url = new URL(req.url || "", `http://${req.headers.host}`); | ||
return new HttpRequest({ | ||
method: req.method, | ||
headers: convertHeaders(req.headers), | ||
query: converQueryString(url.searchParams), | ||
path: url.pathname, | ||
body: req, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not much familiar with NodeJS's http module, so not deeply verifying that use of url and body are appropriate, but seems reasonable, as long as they work in your testing.
29a8c12
to
1eb75dd
Compare
This pull request adds initial (and very basic) sSDK runtime support for NodeJS. This is still missing a ton of testing: It was tested locally using a simple sSDK project.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.