skdb icon indicating copy to clipboard operation
skdb copied to clipboard

Consider moving Context methods to elsewhere in the API

Open jberdine opened this issue 1 year ago • 7 comments

Consider changing the API so that LazyCollection is class with a constructor/operation that takes a Context, instead of having createLazyCollection as a method of the Context interface; and likewise, move useExternalResource to be a constructor/operation of EagerCollection; and jsonExtract could be a floating toplevel function.

The API would be easier to discover and understand if we made those moves. There is not much meaningful that we can write about what the Context is, it is mostly just some opaque type from the user's perspective. That makes it unlikely to be where people go looking for functionality.

The main benefit of the Context is the prevent the call of that functions outside of a map, but if it's a problem, we can hide the context and make that function toplevel.

It seems that this advantage could be retained by transforming calls like context.lazy(Foo, bar) and context.useExternalResource({...}) into LazyCollection.create(context, Foo, bar) and EagerCollection.ofExternal(context, {...}).

jberdine avatar Nov 27 '24 11:11 jberdine

I worked on this a bunch this afternoon, but think we're better off keeping things as they are -- static functions that take a context argument are nice in client code, but require a lot of contortions in our API code, since static methods on abstract classes or interfaces are not allowed and workarounds cause a bit of a mess.

bennostein avatar Dec 12 '24 14:12 bennostein

I'd like to understand better. Is there some reason not to make EagerCollection and LazyCollection into classes? Then they can have static methods, right? Or is that not enough?

jberdine avatar Dec 12 '24 14:12 jberdine

Our current setup has an interface EagerCollection in skipruntime-ts/api and an implementation EagerCollectionImpl in skipruntime-ts/wasm. (and analogous for lazy)

Adding the static method to the implementation class is easy; the problem is that then, if we change those interfaces to abstract classes, we have to (a) add a non-abstract default implementation of the static member, and (b) explicitly include the [sk_frozen] symbol tag.

That would leak a bunch of implementation detail and general clunkiness into api.ts, and introduces some circular dependencies which would need resolving. I got to that point and decided the juice wasn't worth the squeeze. But happy to discuss further if you think the syntactic benefit in clients is worth it!

bennostein avatar Dec 12 '24 14:12 bennostein

But why not make them non-abstract classes? Maybe I'm just not understanding OO. :-)

jberdine avatar Dec 12 '24 14:12 jberdine

Because all of the implementation detail depends on wasm/FFI stuff which we don't want to include in the api package, but we definitely do need the interface there.

bennostein avatar Dec 12 '24 14:12 bennostein

Ok, there seem to be more constraints than I understand.

Finally, what about moving what are currently methods of Context to independent top-level functions? Are there technical issues with that too?

jberdine avatar Dec 12 '24 22:12 jberdine

I'd like to return to this in the context of https://github.com/SkipLabs/skip/issues/622. There, @skiplabsdaniel wrote

Now, api and core can effectively be merged.

and now e.g. EagerCollectionImpl is in core. So I wonder if we were to go ahead and merge api and core, would that enable making EagerCollection a non-abstract class with methods such as useExternalResource?

jberdine avatar Jan 15 '25 12:01 jberdine