Review current API

[rubensworks]

@RubenVerborgh Could you have a look at the state of the repo and the current API? If all looks ok to you, I'll start assigning the creation of new packages to people (such as #4).

[RubenVerborgh]

Thanks @rubensworks, this is really solid work.

I like the main division in packages and functionality, that works well.

Do people have the instructions on how to add new things? Because it might be intimidating to those who have not worked with a monorepo before. This documentation task can be outsourced to one of our colleagues (who can then document their own learning journey).

Major comments

My only major comment is the defaultSolidUtilContext function that needs to be passed around. I understand the necessity and see the benefit of it over having to always configure dependency injection. That said, it looks like it would be a burden to consumers to always have to think about it.

I propose a solution in which we expose primitives that 1) have the default context built in 2) allow to customize the context.

When the primitive is a class, we can just pass { context } as a constructor argument.

When the primitive is a function (as getIdp), I propose to define a creation function withContext as follows:

// Library code
const getIdp = withContext((context: SolidUtilContext) =>
  (webId: string): Promise<string> => {
    // implementation uses `context`
    return webId;
  });

// Consumer code
let idp = getIdp(webId); // just works

const getIdpTraversal = getIdp.with({ engine: myQueryEngine });
idp = getIdpTraversal(webId); // just works

So withContext<C, F> is function a function with 2 arguments:

1. (C => F)
2. C (optional, defaults to defaultSolidUtilContext())

returning F { with: C => F } (but the with is recursive).

Rough draft (with a neat twist where createWith also has with functionality):

const createWithContext = defaultContext =>
  function withContext(createFunc, context = defaultContext) {
    const func = createFunc(context);
    func.with = newContext => withContext(createFunc, newContext);
    return func;
  }

export const withContext = createWithContext(defaultSolidUtilContext())(createWithContext);

Alternatively, instead of just taking newContext, we could merge the passed context with the parent context:

    func.with = newContext => withContext(createFunc, { ...context, ...newContext });

Then it really makes sense to have

const getIdpTraversal = getIdp.with({ engine: myQueryEngine });
const getIdpTraversalSession = getIdpTraversal.with({ session: mySession });

Minor comments

• cd comunica on the README 😉
• (some other small nit but I forgot)

[rubensworks]

The above should definitely be possible, but my only concern is that tree-shaking won't be possible, which may be an issue for users that want to shake out the large default Comunica engine. I initially had a system with default arguments in place, but removed that due to incompat with tree-shaking as well.

I can think a bit more on how to enable tree-shaking with this, but I think that we'd need 2 variants of each package if we really want to enable this.

[RubenVerborgh]

Good point about tree-shaking. But usability is key too.

Could we fix this with different exports? Something like @solidlab/light or @solidlab/lib/light?

So then @solidlab/lib would export getIdp.with({ engine: new ComunicaEngine() }).

And @solidlab/light (@solidlab/lib/light) would export getIdp.with({ engine: new NoEngine() }) or getIdp.with(new ErrorObject()) where the former fails with an error message when a method is called on the engine, and the latter fails with No engine set. when the engine property is accessed?

Then we have both tree shaking and usability.

[rubensworks]

Refactored the code based on the discussion above in e8554cc431ef7e8cdfc31b56b825fb1c1ced5851

I went one step beyond, and also added a type-checking mechanism to ensure that functions can only be invoked after the necessary context fields have been set using .with(). (to avoid runtime crashes when the query engine has not been set)

Could you have another look at this @RubenVerborgh?

[RubenVerborgh]

Ooh that's great, love the type-checking twist!

[RubenVerborgh]

Perfect, thank you!!

[rubensworks]

Ok, then we can go forward with the next steps. Closing this one :-)