Server refactor

[lastmjs]

The main purpose of this PR is to make the server more modular and composable so that the end developer can create hybrid HTTP/Candid canisters easily. Multiple ways to do this are shown and tested.

[bdemann]

What is the feature?

Does it have an issue?

No. It's just a sub point of #1691

What is it doing?

The main purpose of this PR is to make the server more modular and composable
so that the end developer can create hybrid HTTP/Candid canisters easily. Multiple
ways to do this are shown and tested.

Seems straight forward enough

Expectations

Tests

• A canister that is a server with a custom init method
• A canister that is a server with a custom post upgrade method
• A canister that is a server with both an init and post upgrade method
• A canister that is a server with update and query methods
• A canister that is a server with update, query, init and post upgrade methods

More variations than that might be overkill. I think the above cases should be
sufficient.

I expect init, post upgrade and update methods to set some sort of state to
prove that they were called, and have some sort of way to get that out, either
by a server endpoint or a query method.

Code

I think that is all I expect to see and I don't expect this pr to be very in
depth it seems pretty straight forward. I should see a pretty straight forward
way to add these methods that is simple for a user to understand.

The user in this case will be someone with enough IC knowledge to know that
init, post-upgrade, update, and query methods are a thing, so we can expect a
decent amount of IC knowledge when designing the API.

[bdemann]

Results

Code

Things looked pretty good, just a couple of consistency and documentation
concerns.

Is there any benefit into thinking about how I would make the API?

export default Server(
    () => {
        const app = express();

        app.get('/get-endpoint', (req, res) => {
            res.send('Hello world');
        });

        return app.listen();
    },
    {
        init: init(
            [User, Reaction, Principal],
            (initUser, initReaction, initOwner) => {
                user = Some(initUser);
                reaction = Some(initReaction);
                owner = Some(initOwner);
            }
        ),
        pug: post_upgrade(
            [User, Reaction, Principal],
            (initUser, initReaction, initOwner) => {
                user = Some(initUser);
                reaction = Some(initReaction);
                owner = Some(initOwner);
            }
        )
        myUpdate: update([], [], () => {})
        myQuery: query([], [], () => {})
    }
);

I suppose I would do it like that. First with the server callback, then with a
body like Canister that would be optional. Cool, that's what it looks like has
been done!

Tests

Examples

Everything is in one example hybrid_canister.
Hybrid canister has according to the dfx.json 3 canisters

• server
• server_init_and_post_upgrade
• canister

Canister

Ah, canister is testing from the other way around. Instead of asking what if the
server could have canister methods, it's asking what if a canister could have
server endpoints.

Good idea!

That does add another layer to the tests I would hope to see to be exhaustive in
the coverage. I would at least like to see an init method used on the canister
side in addition to using serverCanisterMethods to put them in there. Especially
since it's going to be critical that developers add the setNodeServer call to
their init and post_upgrade methods.

Server

Server is the same as canister and is what I was expecting from the server
perspective.

Server Init and Post Upgrade

Server Init and Post Upgrade is Server but with a init and post upgrade method.
Looks good.

Missing Tests?

Combinations

There are a few combinations that aren't represented that I think would be good to test.

A canister that is a server with:

• a custom init method
• a custom post upgrade method
• both an init and post upgrade method
• update and query methods
• update, query, init and post upgrade methods

Maybe adding all three of the missing ones (just init, just post-upgrade, and
just init and post upgrade with not query or update) is a little overkill, but I
think at least one of them would be beneficial. I would probably at least one of
just init or just post upgrade as a minimum.

Shared Data

I think it could be interesting to test or show the different endpoint/canister
methods working on the same data and not just separate data that is only every
touched by one half of the canister.

Test code

After looking at the test examples I can foresee that the actual tests should
be very straight forward. Each of those three examples have four endpoints or
canister methods, But the post_upgrade tests should be called twice so there
should be a total of 16 tests that should be simply calling the endpoints or
methods and checking the results. I'm not expecting anything complicated here.

I would expect more given my requests above, but given the current state of the
examples that's all I would expect.

Unsurprisingly such an approach would be a little cluttered since we are testing
three independent canisters in one test. I think it would be much easier to
read if the tests where broken up into one test file for each canister and then
each endpoint could have it's own simple test instead of being part of one big
conglomeration of all the endpoints of that canister.

The post upgrade function is not being tested at all.

Which, thinking of the ramifications of that makes me feel more strongly that
we ought to test both a server with just an init override and a server with just
a post upgrade override because I want to be sure that both of those functions
are called regardless of the combination they are in since the call to
setNodeServer is so crucial. Here we don't even deploy it again so we wouldn't
even get the error warning that nodeServer was undefined.

[lastmjs]

I'd like to see this conditional receive the same declarative treatment as #1699, @bdemann wants this on the if statements on server just like the if statements on fetch_ic

Update: this has been resolved

The merge-base changed after approval.