Feedback requested: AsyncConnection and (soon) AsyncManager

[jasononeil]

I would like to (finally) work on getting sys.db.* to work with NodeJS, which of course means making async versions of Connection and Manager (and considering how to handle Object.insert() etc).

Here's what I've done so far:

• Create an AsyncConnection interface
• Create class sys.db.MysqlJs implements AsyncConnection {} using this NPM
• Done some preliminary work in RecordMacros, in preparation for an AsyncManager.
• Refactored a few existing lines in RecordMacros to use reification because it was easy to do and more readable.

What I want my next steps to be:

• Create an AsyncManager (or adapt Manager to have async methods)
• Figure out how to adapt Object to work async.
• Make an async version of TableCreate.exists() and TableCreate.create().
• Make an async version of Transaction.run() and Transaction.isDeadlock().
• Write an async version of the unit tests, make sure they run in Travis
• Clean up the PR, remove the NPM stuff etc. Consider moving MysqlJs class to a separate repo.

But before I go further, I have some decisions to make, and would appreciate feedback:

1. Do we want this in record-macros or as a separate haxelib?

If we want it in a separate haxelib, ignore all further questions and I'll figure it out myself :) But I'd like to merge it in.

2. What approach should I take with the manager?

The existing Manager class has a lot of very old code and covers a lot of edge cases, so I either want to re-use it, or do a clean implementation. My options are:

1. Adapt Manager to expose both sync and async methods. Actually shouldn't be that hard... these are the only two places we need to update (and then the flow-ons to other functions):

   • cnx.lastInsertId() in doInsert()
   • cnx.request(sql) in unsafeExecute()

2. Create a new AsyncManager class, but start from a copy/paste of the old Manager class. Advantage: keep the two separate and a bit cleaner. Disadvantage: code repetition, having to fix bugs in two places etc.

3. Create a new AsyncManager class from scratch, but make it less magical, so no automatic caching etc. I feel there's two many edge cases here for me to cover appropriately in a first build.

3. Object.update, Object.insert, etc.

Should I bother trying to adapt these functions to be async? One idea would be to define the signature like so:

public function insert(?cb:CompletionCallback):Void
public function delete(?cb:CompletionCallback):Void
public function update(?cb:CompletionCallback):Void
public function lock(?cb:CompletionCallback):Void

Where a completion callback is optional, and triggered on both sync and async platforms, but can safely be ignored on sync platforms.

---

Thoughts? Comments? I'm working on this as a side project for the next week and a bit so would love your thoughts

[bablukid]

I'm not a nodejs user but I think it's important to keep this in one single library.

About question 2 : why not having 3 classes, one for sync IO, one for async IO, one for the common logic ( SQL generation...etc )

Currently the Manager class is really fat ( 740 lines ) so it would become more maintainable if concerns are separated. Its typically a case of "god object" antipattern :)

[jasononeil]

Great feedback @bablukid

I've started refactoring by creating a separate BaseManager class which has the core logic, and moving the public interface out to a class Manager extends BaseManager {}, and eventually a class AsyncManager extends BaseManager.

As I begin to unwind this there is a fair bit of logic that is difficult to re-use in an async context, so lots of refactoring may be required, which makes the PR harder to code review. Hopefully it will make the library more maintainable though!

[jasononeil]

I've also realised that sys.db.Connection comes from the standard library, so I will need to eventually open a pull request on HaxeFoundation/haxe/. I'll keep the work in progress here until that is closer to being ready though.

[jasononeil]

Well, I've done it:

• New AsyncManager class that works with Node JS.
• Manager is refactored into separate BaseManager, Manager (sync) and the new AsyncManager class
• All macros adjusted to suit slight changes to API
• Optional async signatures added to sys.db.Object
• All existing neko (sync) tests passing.

I plan to write async versions of the unit tests and run against a real code base. But other than that it would be good to get some more feedback, and see if people are happy to merge :)

[PXshadow]

Currently It's not possible to test on latest haxe versions because #12 has not been merged.

[bablukid]

Thanks for the big work Jason !

Do you think it would be possible to easily migrate synchronous code for neko to nodejs, just by adding @await ?

Like var res = @await db.User.manager.get(1,false);

BTW I heard await will be implemented in haxe 4, do you know more about it ?

[jasononeil]

Closing old/stale PRs