Reworking of the config system #340
Conversation
| throw new Error(`Tried to lookup config that does not exist: ${config}`) | ||
| } | ||
|
|
||
| let c = configs[config]() |
There was a problem hiding this comment.
So, we're fine this will modify the original object whenever clone is set to false? And if so, what will happen if a plugin gets a config that has already modified by it before*? I'm aware this is how it worked originally, but we didn't have plugins modifying the false (yet), then, only core. And we are/were very careful to make sure we clone when we do that... Seems like a can of worms.
* If intended, we should probably have a guideline how a plugin can keep track if its changes has been applied (eg. by adding a __pluginname: true to the root or something which can then be if-checked before applying.
There was a problem hiding this comment.
This will modify the original object even if clone is true. It's a matter of do we want to always clone (very slow for big objects like the H1 templates) or only clone when we know the consumer code is going to modify the object being returned.
There was a problem hiding this comment.
I think in the end it's not really our responsibility. I do think that in the case of clone == true, we should tap the cloned object, not the source object. Since we always pass clone to the tap, it's up to the plugin to handle it gracefully.
I'm trying to think of situations where one might want to apply different logic based on clone, but can't really think of one right now. Maybe a non-issue?
There was a problem hiding this comment.
So I've been thinking on this, maybe we just have configs be modified by plugins when they are first (lazily) loaded? I can't think of any plugins that need to modify a config, say, after a specific server event is fired.
| import * as databaseHandler from "../../components/databaseHandler" | ||
| import * as platformEntitlements from "../../components/platformEntitlements" | ||
| import * as databaseHandler from "../../components/databaseHandler.ts" | ||
| import * as platformEntitlements from "../../components/platformEntitlements.ts" |
There was a problem hiding this comment.
Why do these need a .ts extension? Can't we fix that with a tsconfig.json? Haven't actually pulled yet to play with it, just wondering out loud.
There was a problem hiding this comment.
Basically had to switch the tests over to ESM because of this code right here which needs top-level await to ensure it's done the thing before the tests run, but not import the module too early, because it could cause core code to be initalized without having proper globals installed, creating a whole new can of worms.
There was a problem hiding this comment.
Might have to play with the config a bit, I've had a Vite-based project setup that outputs ESM, but while I could still code in a NodeJS style. Will glance over it again sometime later this week. Maybe there is a better way to bootstrap the whole application for testing purposes...
…340 Signed-off-by: Reece Dunham <[email protected]>
RDIL
changed the title
Fixes #339