-
-
Notifications
You must be signed in to change notification settings - Fork 325
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extracts config.json into its own module #1061
Conversation
5883a50
to
7c70a84
Compare
app/server/mergedServerMain.ts
Outdated
@@ -8,6 +8,7 @@ | |||
import {FlexServer, FlexServerOptions} from 'app/server/lib/FlexServer'; | |||
import {GristLoginSystem} from 'app/server/lib/GristServer'; | |||
import log from 'app/server/lib/log'; | |||
import { getGlobalConfig } from "app/server/lib/globalConfig"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to stop nagging about this now, but, it would be great to copy local style when you can (in this case, imports in alphabetical order, and single quotes, and no spaces near {})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bah, sorry - I'll get better at checking this myself during PR. I keep missing it when the IDE auto-adds an import. 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be sorted 👍
*/ | ||
export async function getGlobalConfig(): Promise<IGristCoreConfig> { | ||
if (!cachedGlobalConfig) { | ||
log.info(`Loading config file from ${globalConfigPath}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe Checking for config file in
rather than Loading
just to be noncommittal about whether one actually exists?
@@ -0,0 +1,107 @@ | |||
import { assert } from 'chai'; | |||
import * as sinon from 'sinon'; | |||
import { ConfigAccessors, createConfigValue, Deps, FileConfig } from "app/server/lib/config"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put the imports in alphabetical order, and keep the quoting locally consistent. If you know I'm going to review, it can be a good idea to pre-review your own code for my import foibles: alphabetical, and consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do! I'm stuck in bad habits, too used to having a linter around that auto-formats all this! 😆
let configObject = { ...input }; | ||
|
||
if (checkers.IGristCoreConfigFileV0.test(configObject)) { | ||
configObject = upgradeV0toV1(configObject); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Migrations! Didn't think of that. Might have been an argument for trying a little harder to stick with the home db, which has a fully worked out migration system, and which can work across different servers without consistency problems. This is an expensive boolean store when you factor in the costs of future devs wrapping their heads around it. Again the hope will be we don't need to work on it much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spoke with Jordi, and ok to land this once you are happy with it.
c26622e
to
0a00bd0
Compare
This adds a config file that's loaded very early on during startup.
It enables us to save/load settings from within Grist's admin panel, that affect the startup of the FlexServer.
The config file loading:
It should be extensible from other versions of Grist (such as desktop), by overriding
getGlobalConfig
in stubs.Some minor refactors needed to occur to make this possible. This includes:
loadConfig
function in FlexServer intoloadLoginSystem
(which is what its main purpose was before).