search

dbos-inc / dbos-transact

The TypeScript framework for backends that scale
https://docs.dbos.dev
MIT License
291 stars 20 forks source link

Configurable Classes & AWS Services (SES, S3) #460

Closed chuck-dbos closed 1 month ago

chuck-dbos commented 1 month ago

This PR is now bigger than any PR should be. As such, it shouldn't be held any longer than necessary. As you review, please consider what is an absolute must - like we just don't like the API and would never approve, vs. what could be dealt with in a future iteration / cleanup pass (unit testing of recovered workflows, debugger, documentation, etc.) That is to say, is the design good? Good meaning we like the API generally, though there may be little bugs hiding in the implementation where some string didn't get passed around?

PR Contents: Configured classes and decorators and such per the design doc. AWS SES communicator (configured) AWS S3 component (communicator and workflows, with example user schema and transactions in the test file) Handling fallout of any unknown unknowns stuff.

I will keep working on tests, and adding better type checks to invocation of child workflows, and adding type template checks to the S3 library.

chuck-dbos commented 1 month ago

Correct me if I was wrong: The main complexity seems to come from we're trying to use instance/configured classes while keeping everything static, for example we have to use initClassConfiguration() instead of just new ClassName().

I am not sure what is precisely meant by "the main complexity". Like I told Max, building around new ClassName() may be possible but you would still have to register the class with a name (so maybe do the construction under a wrapper). You get one configuration per name, not the general-purpose instance state people expect when they say new ClassName(). We would have to find a way to get invoke(object) to basically do what invoke(config) does now w/ a proxy, by inheritance probably so that we had a name field available, or maybe we could look it up. This might help with typing a little bit on getting the config info. Do we want to build a whole new prototype that way for comparison?

qianl15 commented 1 month ago

We would have to find a way to get invoke(object) to basically do what invoke(config) does now w/ a proxy, by inheritance probably so that we had a name field available, or maybe we could look it up. This might help with typing a little bit on getting the config info. Do we want to build a whole new prototype that way for comparison?

Building around new ClassName() doesn't sound simpler to me.. I think your current implementation is a good one given the constraints 1) static functions only and 2) rely on context to access config information.

demetris-manikas commented 1 month ago

@chuck-dbos I see that you have made changes in the user_database.ts which I have touched as well in #450 If you like what I have done there it would be nice if you incorporated my changes so that I won't have to deal with merge conflicts in this crucial file. If not no problem I will go your way. The transaction part is probably a correct change and if so it should be incorporated ASAP.

chuck-dbos commented 1 month ago

Yep, I'm on break today but will merge the conflicts soon after.  Has been taking 90 minutes but hopefully only 60 this time since it is broken up a bit.

chuck-dbos commented 1 month ago

This may have already been answered, but are you working on unit tests for this? The only tests I see are for the two AWS packages, and as far as I can tell those don't run by default. Would love some tests in the main package since this is so complex.

Correct.