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

Type definition for tables()._db, tables()._doc is wrong for nodejs18.x runtime #540

Open
lpsinger opened this issue Dec 16, 2022 · 10 comments

Comments

@lpsinger
Copy link

Describe the issue
The type definitions for tables()._db and tables()._doc are wrong if the application is using the nodejs18.x AWS Lambda runtime. The types should come from the AWS SDK v2 if using the nodejs16.x runtime or earlier, and the AWSD SDK v3 if using the nodejs18.x runtime or later.

lpsinger added a commit to lpsinger/functions that referenced this issue Dec 16, 2022
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.
lpsinger added a commit to lpsinger/gcn.nasa.gov that referenced this issue Dec 17, 2022
lpsinger added a commit to lpsinger/gcn.nasa.gov that referenced this issue Dec 19, 2022
lpsinger added a commit to lpsinger/gcn.nasa.gov that referenced this issue Dec 19, 2022
lpsinger added a commit to lpsinger/gcn.nasa.gov that referenced this issue Dec 19, 2022
lpsinger added a commit to nasa-gcn/gcn.nasa.gov that referenced this issue Dec 19, 2022
@TheWarBadger
Copy link

I've just lost a chunk of time to this - pretty knotty thing to fix but at the very least it would be useful to add a comment to tables.d.ts to indicate that _db and _doc may be the incorrect type depending on the version of node in use.

Happy to chip in with the fixes if that'd be useful.

@ryanblock
Copy link
Member

@TheWarBadger @lpsinger PRs def welcome! /cc @tbeseda

@tbeseda
Copy link
Member

tbeseda commented Feb 23, 2023

Oh yikes. I hadn't thought about these types inherently changing based on the Node runtime.
Architect functions provides compat between AWS SDK v2 and v3 based on what's available in the Lambda now.

I don't use these types on the daily, so any ideas around how we might change the usage pattern here is welcome. Like is it easier to manually annotate types from specific imported types? as opposed to the implicit types provided when referencing arc.tables()

Or maybe arc/fns implicitly types with SDK v3 but provides v2-based types that can be imported for handlers in Node14-16?

@lpsinger
Copy link
Author

What about providing two different type packages, one for AWS SDK v2 and one for AWS SDK v3?

@lpsinger
Copy link
Author

Or templating the types so that you can do await tables<2>() or await tables<3>?

@TheWarBadger
Copy link

@Ipsinger sounds good, I would suggest extending the API with some sensible run time protections - E.G. tables_sdk3() (or some Template equivalent) which throws a runtime exception with a useful message like trying to use AWS sdkv2 but Architect uses v3 when running on Node18. That way if someone upgrades their node version they get an immediate sensible error (including an unspecified version for people who don’t need the lower level API).

That said, I won’t be able to check this for a couple of days due to being AFK, but if using a type Union of DynamoV1 | DynamoV2 (class names are wrong but you get the idea) for the existing _doc etc is feasible then it would force the user to do their own type guards. Not as elegant but also gets the job done.

@lpsinger
Copy link
Author

@Ipsinger sounds good, I would suggest extending the API with some sensible run time protections - E.G. tables_sdk3() (or some Template equivalent) which throws a runtime exception with a useful message like trying to use AWS sdkv2 but Architect uses v3 when running on Node18.

What if the user is using a custom Lambda runtime that has node 16.x but the AWS SDK v3 installed? It would be better to attempt to import the V3 SDK and fall back to the V2 SDK if it is not installed.

@ryanblock
Copy link
Member

What if the user is using a custom Lambda runtime that has node 16.x but the AWS SDK v3 installed?

I would say that is not a use case we currently intend to support. It could be done, but it would add a fair bit of complexity for something that is, as far as I can tell, theoretical. I've yet to field a report of an Architect user doing this fwiw!

@lpsinger
Copy link
Author

A custom Lambda runtime is something that I have contemplated with my project.

@ryanblock
Copy link
Member

Use of Architect Functions in custom runtimes is totally fine and an intended use case! I'm referring specifically to a custom runtime where:

  1. The version of Node.js in 16 (which is maintenance / scheduled to go EOL in a few months), and
  2. It's paired with the current version of the AWS SDK (v3), which would not conform to the Lambda's convention of pairing Node + SDK versions in production (<=16 = SDK v2, >=18 = SDK v3)

While inadvisable, there nothing fundamentally impossible or wrong with that approach, but it is an obscure set of circumstances that I don't think we need to support.

Probably more likely would be a custom runtime that uses the active version of Node.js (18), or a higher version, that would be paired with the current AWS SDK (v3).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants