Enable dependency injection with auto-generated components #74
Conversation
|
Added developer docs here: https://github.com/solid/community-server/wiki/Notes-for-developers#add-components-to-configuration |
|
Fix for Windows is pushed. |
There was a problem hiding this comment.
This automated config generation thing is great. Really would have liked this when we were making Comunica :D .
Regarding the structure: I think we probably want to structure it in such way that the things that are most likely to be replaced by something are in separete files. Not 100% sure what those would be, but my guess would be all the separate inputs of the AuthenticatedLdpHandler (so would put each of those big blocks in a separate file, and for example add the acl authorization to the acl.json). The stores are probably another place more likely to have changes but that one's a bit more difficult since we have a stack of stores. Perhaps put the actual store handling the data in a separate config? But then again the sparql store is not going to need some of the patching handlers, so not sure there.
| }) | ||
| .help(); | ||
|
|
||
| new Promise<RuntimeConfig>(async(resolve): Promise<void> => { |
There was a problem hiding this comment.
This might have something to do with this being CLI stuff and async is a problem, but why is a new Promise made here? Because it seems like everything in there could also just be in a try/catch block with 1 await in there.
There was a problem hiding this comment.
We need runCustom to be non-async, because we want to call it from bin/ without having to add a then/catch there.
There was a problem hiding this comment.
But it's fake safety because it's completing non-synchronously anyway.
There was a problem hiding this comment.
It's safe though, since we handle then and catch directly here.
But in any case, I can remove the explicit Promise constructor as you've mentioned, and just call the async function directly.
Sure, that might be better indeed! Will change. |
rubensworks
force-pushed
the
feature/components
branch
from
af36beb
to
466539f
Compare
|
Rebased on master, so this one should be good to go. |
| const loader = new Loader(properties); | ||
| await loader.registerAvailableModuleResources(); | ||
| const setup: Setup = await loader | ||
| .instantiateFromUrl('urn:solid-server:my', configPath); |
There was a problem hiding this comment.
@rubensworks Still a my here, is that a problem?
There was a problem hiding this comment.
Yes, it looks I didn't test things properly.
We probably want some automated tests around this bin...
There was a problem hiding this comment.
It's a class now, so we don't even need bin tests. But a system test that boots up the bin would be needed indeed.
While this PR may seem fairly simple, most of the work for this was spent in this new tool: https://github.com/LinkedSoftwareDependencies/Components-Generator.js
Every time you now run the
buildscripts, thecomponentsfolder will automatically be filled with.jsonldfiles based on all the exported TypeScript files (using Components-Generator.js). These components are used within our declarative dependency injection configuration (seeconfig/).I already split up the config file over different presets, which will make it easier to modularize in the future. Suggestions for structuring the config differently are definitely welcome.
It's very important we get this part right, so please review in detail :-)
I will also write some based dev documentation, so people know how to work with this.