michielbdejong / wac-ldp-kit

A resource store for use in LDP-like servers such as Solid servers. Also usable as LDP server.
MIT License
1 stars 0 forks source link

[WiP] LDP functionality #1

Closed michielbdejong closed 5 years ago

michielbdejong commented 5 years ago

Comparison:

RubenVerborgh commented 5 years ago

That’s just awesome 👏

michielbdejong commented 5 years ago

comment to test CLA setup

michielbdejong commented 5 years ago

Drastically simplified the code architecture, so that there is still a concept of workers, but they don't pass messages on to the next worker; instead, they expose a handler that returns a result or throws an error.

39 tests passing, and counting.

will keep working on this for the rest of the week and then submit as a pull request to a repo on the inrupt org. Thinking of calling it wac-ldp-kit.

RubenVerborgh commented 5 years ago

Why the workers though? Is there a discussion on necessity/advantages/disadvantages?

michielbdejong commented 5 years ago

The workers came out of two discussions, one about how we can achieve the highest level of modularity, so that for instance @kjetilk's Auth* Proxy idea is also implementable by simply connecting a shuffled set of workers together. Each worker could in theory be an npm module, although they would only have value in combination with the other ones, so that's why I thought I might as well keep them in one repo.

The other discussion was about the idea of microservices, but simplified because these workers live in the same process and there's no message queueing. So since each of my workers comes with a Task type and a Result type, and the worker itself is stateless and only exposes one function that converts a Task into a Result, it should be easy to write unit test for such a worker and reason about how it works.

I went through two experimental variations and I hope you agree that the third one, here, is a lot better than the previous versions, and that it's now a nice balance between straight-forward but still enjoying the benefits of high modularity. At least I'm starting to be pretty happy with it now. :)

kjetilk commented 5 years ago

This sounds really great!

I think I would still prefer to have those that are relevant to the proxy in a separate npm package, so that you would only need to install the ones that you'd actually use though.

RubenVerborgh commented 5 years ago

@michielbdejong Thanks for explaining.

However, if I may be critical (and I take that to be my job), then I must say that none of the listed benefits are a consequence of the worker/task-based design.

So I currently only see the extra complexity created by worker/task (= two classes for everything, and things need to be worker-shaped instead of taking the most natural form for the thing to implement), but no additional benefits. Having ResourceDeleter, ResourceUpdater, etc. imposes a very specific architecture, but I don't think we have sufficient arguments to justify it.

Perhaps in a concrete question: the task/worker paradigm complexifies the code considerably, but for what gains that cannot (easily) be achieved without it?

michielbdejong commented 5 years ago

To do:

Follow-up:

CLAassistant commented 5 years ago

CLA assistant check
All committers have signed the CLA.

michielbdejong commented 5 years ago

first draft ready and mostly working, will read through the whole PR myself once more now and then submit this for review.

michielbdejong commented 5 years ago

Split into https://github.com/inrupt/wac-ldp/pull/6 https://github.com/inrupt/wac-ldp/pull/7 and https://github.com/inrupt/wac-ldp/pull/8