Add a script to generate a sample config #8899
Conversation
There was a problem hiding this comment.
Overall, I agree improving the sample config to better describe available options is a good thing to do. Beyond just making the keys appear in the sample, having text to explain them would be even better. Anyway, I am generally positive on the idea and the implementation here.
| @@ -1,34 +1,120 @@ | |||
| { | |||
There was a problem hiding this comment.
I really wish we could parse the config in such a way that allows inline comments. I think the sample file would be improved even more (especially if we do list every default value like this) if we could:
- Clearly state that these are defaults
- Comment them out, because they are defaults and don't need to be set
- Describe the purpose of each setting in a comment in the sample file
- This could be done by adding
descriptionkey or similar toSettings.jswhich is extracted here as a comment
- This could be done by adding
There was a problem hiding this comment.
JSON can't have comments :(
There was a problem hiding this comment.
if we used YAML or something, I'd definitely be including descriptions for all of this. I couldn't think of a nice way to actually put a comment in here without people thinking that it is a config option itself.
There was a problem hiding this comment.
Right, JSON can't have comments (which annoys me for most JSON-based configuration). Sorry, I should been more specific...
Along with YAML, other posibilities include:
- Cheat and still call it "JSON", but fetch it as text, strip out comments, then hand it
JSON.parse(ESLint does this for*.jsonconfig files) - Rename to
config.jsand have the config file export an object (which implicitly allows comments by allowing full JS syntax)
There was a problem hiding this comment.
I'm moderately in favour of calling it """"""JSON""""""
(or we take up HCL)
There was a problem hiding this comment.
note to self: replace https://github.com/vector-im/riot-web#configjson with comments, as mentioned above
There was a problem hiding this comment.
@turt2live @jryans one way to have inline comments might be to allow json5 as a second format of the json: https://json5.org/
But as that needs more changes to the core itself I guess that should (If wanted) go into an extra PR
| const setting = settings[settingName]; | ||
|
|
||
| if (setting.isFeature) { | ||
| phasedRollOut[settingName] = { |
There was a problem hiding this comment.
It feels a bit like overkill to print this phased roll out bit for every feature. Maybe just the first?
| }; | ||
|
|
||
| function parseSettings() { | ||
| // This is by far the worst and cleanest way to load the settings config. |
There was a problem hiding this comment.
Hmm, agreed this is unfortunate... Is there any way to avoid the eval? Some possibilities:
- Move
SETTINGSobject out ofSettings.jswithout the controllers and such (not great, since likely means bits of settings in multiple places) - This script could set an environments variable, and then
Settings.jscould check this at import time and replace dependencies with no-op stubs to allow a normalrequireto complete
| const fs = require("fs"); | ||
|
|
||
| const SAMPLES = { | ||
| default_server_name: "matrix.org", |
There was a problem hiding this comment.
I haven't decided yet if this sample info is better here or inline in Settings.js with each setting... I just worry it might get forgotten over here, but I suppose the idea is you'd run this script whenever a new setting is added, so hopefully not forgotten then...
There was a problem hiding this comment.
The problem is also that the config is more than just settings. Personally, I'd rather us fill out the defaults for SdkConfig so we don't need this here, but that raises problems. The first problem being that default_server_name or default_hs_url (and is_url) can be defined - it's hard to represent that in a config.
|
If you use riot-web as a release tar.gz file from github, it's just a heap of static files, |
This script in this PR that generates the sample config would be re-run by Riot developers when new settings are added. The script is not meant to be run during deployment. (Or at least, that's certainly how I assumed it would work!) |
|
As @jryans says, it's supposed to be run when new stuff is added, not every release/update/deployment. I also plan to add comments to the thing at some point, because that's what people seem to want. The auto-generation might end up disappearing at the same time because encouraging developers to document their new settings seems like a good thing. |
|
Great! |
|
This approach is clearly not the preferred way of doing this, so closing for now. If we decide we want such a script, someone in the modern era might be able to take care of it. |
Addresses #8449
Requires matrix-org/matrix-react-sdk#2687
This might not be the way we want to actually do this. In my opinion, it's important for us to have a sample config with as many options set as possible, however others might feel differently about that. This also goes a step further and generates a config based on several objects, which also might be questionable.
Feedback is appreciated on whether this is even in the right direction for what we want, and if it actually solves any problems. I've flagged it as WIP for this reason.