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

WIP: Fix types for AWS SDK v3 #541

Closed
wants to merge 1 commit into from

Conversation

lpsinger
Copy link

This is an incomplete solution, because this makes types correct for the nodejs18.x runtime but incorrect for nodejs16.x and earlier.

See #540.

Thank you for helping out! ✨

We really appreciate your commitment to improving Architect

To maintain a high standard of quality in our releases, before merging every pull request we ask that you've completed the following:

  • Forked the repo and created your branch from main
  • Made sure tests pass (run npm it from the repo root)
  • Expanded test coverage related to your changes:
    • Added and/or updated unit tests (if appropriate)
    • Added and/or updated integration tests (if appropriate)
  • Updated relevant documentation:
  • Summarized your changes in changelog.md
  • Linked to any related issues, PRs, etc. below that may relate to, consume, or necessitate these changes

Please also be sure to completed the CLA (if you haven't already).

Learn more about contributing to Architect here.

Thanks again!

This is an incomplete solution, because this makes types correct
for the nodejs18.x runtime but incorrect for nodejs16.x and earlier.

See architect#540.
@ryanblock
Copy link
Member

Superseded by the aws-lite-based version 8!

@ryanblock ryanblock closed this Feb 9, 2024
@lpsinger
Copy link
Author

lpsinger commented Feb 9, 2024

We'd really rather use the first-party AWS SDK. Has Architect dropped support for that?

@ryanblock
Copy link
Member

Architect doesn't have an opinion on AWS SDK, definitely use it if you need it or like it! To improve performance, Architect Functions v8 and going into the future no longer uses AWS SDK by default, although tables() can still instantiate AWS SDK-based DynamoDB clients. Please see https://arc.codes/docs/en/about/upgrade-guide#architect-10-%E2%86%92-11

Code depending on data._db or data._doc must now instantiate with the awsSdkClient boolean option, like so: await arc.tables({ awsSdkClient: true })
Using the awsSdkClient option necessitates having AWS SDK v2 or v3 installed, according to your Lambda’s Node.js version (v2 for 16.x, v3 for 20.x+)

@lpsinger
Copy link
Author

lpsinger commented Feb 9, 2024

I see. As a user of Architect I would have preferred at least one major release where this is an opt-in feature rather than immediately making it the default.

In any case, due to the logic that selects between the v2 or v3 API, the types are still inaccurate. So how is this issue resolved?

@ryanblock
Copy link
Member

Unfortunately, a transition period was neither technically feasible nor desirable. The reason we upgraded Functions in this way is specifically because SDK v2 is being fully deprecated from Lambda in the coming months, leaving us stuck on v3, which is extremely slow. With aws-lite, performance is now 2 to 5x faster for core Arc Functions use cases, like instantiating a Dynamo client.

Anyhow, the v2 / v3 selection logic naturally goes away as v2 / nodejs16.x are deprecated, which means the only v3 related things left to type are ._doc and .db, which we do still provide backward compatibility for. But I did notice we're no longer actually typing those methods, so we should probably take care of that in a new PR.

All other changes from 7 to 8 should be under the hood, so folks can expect 7 will probably continue to work indefinitely. I do strongly advise making use of v8 if all you're doing is using the built in tables methods, though – and those are typed, btw!

For additional background, here's the work @tbeseda did that largely supersedes this PR: 1929c95

@lpsinger lpsinger deleted the aws-sdk-v3-types branch February 10, 2024 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants