dolittle / JavaScript.SDK

Dolittle JavaScript SDK
https://dolittle.io
MIT License
5 stars 2 forks source link

Rename client building/configuring methods #24

Closed joelhoisko closed 4 years ago

joelhoisko commented 4 years ago

This PR renames and reworks some of the builder methods used in setting up a client, creating event handlers/filter etc.

Naming for most of these comes from my proposed glossary (not to be taken too seriously, here to give context for some of my thinking):

This isn't always reflected in everything (eg. we have Client.configureLogging() instead of Client.withLogging()) but it's how I've designed/thought about the naming. Feedback is appreciated!

Also fixed different Id (ArtifactId/PartitionId etc) classes to handle themselves their own type in the static from() method.

Here's an example of configuring everything and sending out an event:

const client = Client
    .forMicroservice('a14bb24e-51f3-4d83-9eba-44c4cffe6bb9')
    .withVersion(Version.first)
    .withEnvironment('dev-test')
    .connectToRuntime('localhost', 50055)
    .withContainer(MyContainer)
    .withCancellation(Cancellation.default)
    .configureLogging(logger => {
        logger.level = 'debug';
        logger.exitOnError = true;
    })
    .withEventTypes(eventTypesBuilder => {
        eventTypesBuilder.associate(MyEvent, '8e538194-3e7b-4bd9-96d6-3d5eeb388a85');
    })
    .withEventHandlers(handlersBuilder => {
        handlersBuilder
            .createEventHandler('02612fa7-e705-40bd-95f1-6b97b92cafc3', handler => {
                handler
                    .partitioned()
                    .inScope('bd86fd8c-3067-4dfb-93f2-d408d5fa6a2f')
                    .unpartitioned()
                    .handle(MyEvent, (event, context) => {
                        return new Promise((resolve) => {
                            console.log('Start processing', event);
                            setTimeout(() => {
                                console.log('Done processing', event);
                                resolve();
                            }, 2000);
                        });
                    });
            });
        // 7H3 1337 h4xoR W4Y
        handlersBuilder.createEventHandler('14d21382-d2a9-4961-b4d5-b56cfa3bc874', _ => _.handle(MyEvent, (evt, ctx) => console.log(evt)));
    })
    .withFilters(filtersBuilder => {
        filtersBuilder
            .createPrivateFilter('79e12ab3-2751-47e1-b959-d898dc4d6ee8', filter => {
                filter
                    .handle((event: any, context: EventContext) => {
                        return new Promise((resolve, reject) => {
                            if (event) resolve(true);
                        });
                    });
            });
        filtersBuilder
            .createPrivateFilter('79e12ab3-2751-47e1-b959-d898dc4d6ee8', filter => {
                filter
                    .inScope('3724a3db-5d00-4a8e-915d-94293d9c1a05')
                    .partitioned()
                    .handle((event: any, context: EventContext) => {
                        return new PartitionedFilterResult(true, Guid.create());
                    });
            });
        filtersBuilder
            .createPublicFilter('2c087657-b318-40b1-ae92-a400de44e507', filter => {
                filter
                    .handle((event: any, context: EventContext) => {
                        return new PartitionedFilterResult(true, Guid.create());
                    });
            });
    })
    .withEventHorizons(eventHorizon => {
        eventHorizon.
            forTenant('900893e7-c4cc-4873-8032-884e965e4b97', tenantSubscriptionBuilder => {
                tenantSubscriptionBuilder
                    .forProducerMicroservice('7a6155dd-9109-4488-8f6f-c57fe4b65bfb', subscription => {
                        subscription
                            .fromProducerTenant('900893e7-c4cc-4873-8032-884e965e4b97')
                            .fromProducerStream('2c087657-b318-40b1-ae92-a400de44e507')
                            .fromProducerPartition('b8e62b8b-6a53-4b4c-b3b4-b8b8aea37c81')
                            .toScope('406d6473-7cc9-44a6-a55f-775c1021d957')
                            .onSuccess((t, s, sr) => console.log('Subscription: Success'))
                            .onFailure((t, s, sr) => console.log(`Subscription: Failed - ${sr.failure?.reason}`))
                            .onCompleted((t, s, sr) => console.log(`Subscription: Completed`));
                    })
                .onSuccess((t, s, sr) => console.log('Tenant: Success'))
                .onFailure((t, s, sr) => console.log(`Tenant: Failed - ${sr.failure?.reason}`))
                .onCompleted((t, s, sr) => console.log(`Tenant: Completed`));
            })
        .onSuccess((t, s, sr) => console.log('EventHorizons: Success'))
        .onFailure((t, s, sr) => console.log(`EventHorizons: Failed - ${sr.failure?.reason}`))
        .onCompleted((t, s, sr) => console.log(`EventHorizons: Completed`));
    })
    .build();

client.executionContextManager.forTenant('900893e7-c4cc-4873-8032-884e965e4b97');
const event = new MyEvent();
event.anInteger = 42;
event.aString = 'Forty two';
client.eventStore.commitPublic(event, 'd8cb7301-4bec-4451-a72b-2db53c6dc05d');

And a nice diagram too: image

einari commented 4 years ago

Should we keep useContainer() or usingContainer() instead of changing to withContainer()?

Maybe container is even not unambiguous enough.

woksin commented 4 years ago

Kinda off topic, but I don't really get the need to have a container in the SDK. Are there any technical reasons for it? @einari

Since in the context of a Client it is hard to have an implicit understanding of what Container means and refers to.

joelhoisko commented 4 years ago

I like the use of word with when the context is that the user is creating the Client. It flows nicely: "create a client with container/version/environment/eventhandler". Also in the code sense the ClientBuilder isn't doing anything special with the Container, just setting it to a property right?

Also "with" chains together nicely: Client.withVersion().withEnvironment().withFilters() makes it clear that Version/Environment/Filters all belong to the Client/modify the Client. With the use/using wording the chain gets broken: Client.withVersion().using/useContainer().withFilters() makes it sound like the the Container has the filters now. Ofc if we change everything to the word use then it works: Client.useVersion().useFilters().useEnvironment() but the word use just doesn't flow as nicely in my head.

This same problem pops up with Client.connectToRuntime().withVersion() where the Version suddenly sounds like to be an act on Runtime so maybe that one could be changed also to something else that sound nicer when chaining? Some examples:

// the most explicit to divide it in 2 parts but a bit wieldy maybe? also the word 'host' could be ambigious?
withHost(host).withPort(port)
withRuntimeAddress(host, port)
withHost(host, port)
withRuntime(host, port)
withConnection(host, port)
withAddress(host, port)
einari commented 4 years ago

@woksin: The container is not for any internal constructions in the SDK - it is purely used right now to be able to instantiate event handlers. It is completely optional, and if you don't have a container - it will use the default implementation which is basically going to new it up.

einari commented 4 years ago

I think the API problem here is that it is all flat and these things sit on the top level, while in fact they are somewhat unrelated and for different purposes. I think for now, this is fine. But I think an improvement could be where you get to something like this:

Client.create(_ => _
   .forMicroservice('... some id...', ms => ms
      .withName('some name')
      .withVersion(...))
   .connectTo('host', 'port')
   .objectCreation(oc => oc
      .usingContainer(Container)
});

One of the things one commonly want to achieve with a fluent API is that it is unambiguous when using it and you can read it as regular english. So wording matters a lot and the meaning behind it.

When everything is on the ClientBuilder- I kinda agree with you, because it looks like you're building the client. While if we had a hierarchy here, you'd be building different things. Like the Microservice, the ObjectFactory.

Anywho, in the interest of moving forward - I suggest we just go for whats already here and then iterate on the fluent stuff. Found a Pluralsight course on Fluent API design - haven't looked at it, but could be worth looking at: https://www.pluralsight.com/courses/designing-fluent-apis-c-sharp

joelhoisko commented 4 years ago

I changed some things:

Also please comment if the EventStoreBuilder.build() method is a bit too grande, I got a bit carried away with tuples.

Example of new stuff:

const client = Client
    .create()
    .forEnvironment('test')
    .withLogging(logBuilder => {
        logBuilder
            // we could have different loggers here or the user could specify their own
            .useWinston(winston => {
                winston.level = 'debug';
            });
    })
    .forMicroservice('7a6155dd-9109-4488-8f6f-c57fe4b65bfb', microservice => {
        microservice.withVersion(Version.first);
    })
    .withEventStore(eventStore => {
        eventStore.withEventHandlers(builder => {
            builder.createEventHandler('fb45d585-d17e-419f-b1ea-8b8382210d61', handler => {
                handler
                    .partitioned()
                    .inScope('a66a9fe7-e346-42a6-9cf3-8cba820f5da4')
                    .handle('MyEvent', (event) => console.log('event handler'));
            });
        });
        eventStore.withEventTypes(_ => {
            _.associate(MyEvent, '104e2285-289b-4dcd-bcf0-a3bb2636d1ab');
        });
        eventStore.withFilters(filterBuilder => {
            filterBuilder.createPrivateFilter('79e12ab3-2751-47e1-b959-d898dc4d6ee8', fb => {
                fb
                    .handle((event: any, context: EventContext) => {
                        return new Promise((resolve, reject) => {
                            console.log('Filtering event', event);
                        });
                    });
            });
            filterBuilder.createPublicFilter('2c087657-b318-40b1-ae92-a400de44e507', fb => {
                fb
                    .handle((event: any, context: EventContext) => {
                        return new PartitionedFilterResult(true, PartitionId.unspecified);
                    });
            });
        });
    })
    .build();