refactor JsonLdOptions to take document loader factory instead of loader

[damooo]

Addresses #143

[pchampin]

thanks a lot @damooo .

A few questions:

• is the LoaderFactory trait really needed? Could we not simply make JsonLdOptions generic over <LF: Fn() -> L, L: Loader<...>> ?? I realize that it may have usability problems, but did you consider it?

• if LoaderFactory is a better option, then do we really need the two specific implementations (DefaultLoaderFactory and ClosureLoaderFactory) of this type? I would rather have a blanket implementation of LoaderFactory for all F: Fn() -> L where L: Loader<...>, so that any closure/function returning a loader would implement it. Note that MyLoader::default would also fall into that category.

[damooo]

is the LoaderFactory trait really needed? Could we not simply make JsonLdOptions generic over <LF: Fn() -> L, L: Loader<...>> ?? I realize that it may have usability problems, but did you consider it?

Yes. I first considered that approach. But there arised an issue with ClosureLoader usage at

sophia_rs/resource/src/loader/_trait.rs

Lines 57 to 68 in 0c38774

	.with_document_loader(ClosureLoader::new(|url| {
	async move {
	let (content, ctype) =
	self.get(url.as_ref()).map_err(|e| e.to_string())?;
	if ctype == "application/ld+json" {
	String::from_utf8(content).map_err(|e| e.to_string())
	} else {
	Err(format!("{url} is not JSON-LD: {ctype}"))
	}
	}
	.boxed()
	}));

That is, there seems also cases, where the closure-loader captures self reference to enable recursion there. Thus to support such non-static lifetimes for the yielded loaders, the factory interface must enable lending factories also. That's why there is a GAT for the loader in the trait.

Also, there was still a possibility to not introduce a trait, but then we have to propagate lifetime generic all the way from jsonld-options to jsonld-parser, jsonld-serializer. That seems a bit disruptive. You can see that approach tried in this branch. The trait will stop such lifetime propagation with a GAT.

Also trait removes the need to articulate many-many bounds on L generic for loader everywhere as in

sophia_rs/jsonld/src/parser.rs

Lines 64 to 73 in 0c38774

	pub fn parse_json(&self, data: &RemoteDocument<ArcIri>) -> JsonLdQuadSource
	where
	L: Loader<ArcIri, Location<ArcIri>>
	+ json_ld::ContextLoader<ArcIri, Location<ArcIri>>
	+ Send
	+ Sync,
	L::Output: Into<Value<Location<ArcIri>>>,
	L::Error: Display + Send,
	L::Context: Into<json_ld::syntax::context::Value<Location<ArcIri>>>,
	L::ContextError: Display + Send,

, with bounds encapsulated by GAT of LF.

if LoaderFactory is a better option, then do we really need the two specific implementations (DefaultLoaderFactory and ClosureLoaderFactory) of this type?

Essentially, only ClosureLoaderFactory suffices. But DefaultLoaderFactory was added to support #[derive(Default)] attributes easily. And it also enables to name the concrete type for default cases. That enabled it to maintain all existing flows properly.

I would rather have a blanket implementation of LoaderFactory for all F: Fn() -> L where L: Loader<...>, so that any closure/function returning a loader would implement it. Note that MyLoader::default would also fall into that category.

Closure factory added to properly support lending-factory-closures. I thought It will be parallel to design of existing ClosureLoader

[pchampin]

all this makes a lot of sense, you convinced me :)

May I suggest, however, to change the signature of with_loader_factory to expect a closure/function rather than a full-fledged LoaderFactory? The methodwould wrap it in a ClosureLoaderFactory internally. This would improve the usability of the method, IMO.

(and people could still pass SomeLoader::default if they want -- it does not really matter at this point whether we use ClosureLoaderFactory or DefaultLoaderFactory in this case)

[damooo]

(and people could still pass SomeLoader::default if they want -- it does not really matter at this point whether we use ClosureLoaderFactory or DefaultLoaderFactory in this case)

They are not the only ones? For example i will be using a named struct called CloningLoaderFactory, that have to be instantiated with a template loader, which will be cloned onwards. Then we cannot supply this factory instance?

Could we have a seperate method instead?

Also, it may be useful if we also provide named struct CloningLoaderFactory by default, which covers most of the cases, so that users can name it in field types if needed.

[pchampin]

For example i will be using a named struct called CloningLoaderFactory, that have to be instantiated with a template loader, which will be cloned onwards. Then we cannot supply this factory instance?

You could still pass the following closure:

move || templateLoader.clone()

and get exactly the same result, couldn't you?

[damooo]

Yes, but what if we want to "name" the concrete type for generic? for type of a field in a struct?

For example, a struct, that includes parser field in it like following:

struct SomeConfig {
  jsonld_parser: JsonLdParser<CloningLoaderFactory<CachingHttpLoader>>,
   a: String,
   b: u32,
}

If we use ClosureLoaderFactory, we cannot name the type of jsonld_parser field in the struct, as closure's type is anonymous, and cannot be named.

[pchampin]

If we use ClosureLoaderFactory, we cannot name the type of jsonld_parser field in the struct, as closure's type is anonymous, and cannot be named.

That's right, you convinced again :)

[pchampin]

I have rebased your PR (77600a2) and made a few additions (436e0e8, 54a84e6, e2d3df6), so this PR is essentially merged, even though GitHub does not recognized it. Thanks again for your work.

[damooo]

_/\_