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

Add sSDK runtime support for NodeJS. #703

Merged
merged 1 commit into from
Mar 6, 2023

Conversation

pose
Copy link
Contributor

@pose pose commented Mar 1, 2023

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.

@pose pose requested review from a team as code owners March 1, 2023 18:59
Copy link
Contributor

@gosar gosar left a 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.
Copy link
Contributor

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

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
 */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the files to use it. @gosar @srchase let me know if that's fine.

smithy-typescript-ssdk-libs/server-node/README.md Outdated Show resolved Hide resolved
smithy-typescript-ssdk-libs/server-node/src/node.ts Outdated Show resolved Hide resolved
Comment on lines 30 to 28
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,
});
Copy link
Contributor

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.

smithy-typescript-ssdk-libs/server-node/package.json Outdated Show resolved Hide resolved
@pose pose force-pushed the features/node-server-support branch from 29a8c12 to 1eb75dd Compare March 2, 2023 16:45
@srchase srchase merged commit 2805675 into smithy-lang:main Mar 6, 2023
@pose pose deleted the features/node-server-support branch March 6, 2023 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants