Conversation
| if (!config.id || !config.telemetry) { | ||
| // If no id OR telemetry, | ||
| // still write the file with a new ID and/or telemetry | ||
| fs.writeFileSync(configPath, JSON.stringify({ |
There was a problem hiding this comment.
Should we worry at this point that we're about to overwrite a file that isn't a CLI config and error out?
There was a problem hiding this comment.
I don't know that it's a huge concern...
I kind of want to keep the directory flexible in this function, but we could make it less likely to be an issue of wiping something non-cli related by refactoring the config file to be named fa.config.json or faconfig.json to make it more intentional?
Looking at this now, we probably also want to keep any additional data intact for future proofing by spreading the rest of the data from the object (if it exists)
| } | ||
| } | ||
|
|
||
| type ConfigObject = { |
There was a problem hiding this comment.
If we decide to change the format of this file at some point we'll want to put a version number in here. Not necessary to do it now, unless you want to indicate to future devs that this is the intent and that we have the pattern established.
There was a problem hiding this comment.
Just so I'm clear (the referenced code lines are odd), you're suggesting versioning the utils file or the config file?
|
|
||
| } | ||
|
|
||
| export function createConfig(dir: string, configObject: ConfigObject = { id: randomUUID(), telemetry: true}) { |
There was a problem hiding this comment.
Can you default dir to ./ since it's referenced in a few places below?
There was a problem hiding this comment.
Does it make sense to have a default or require it to be added?
If we want to default, I think we may want it to default to ./dist/.fa/ since that's the main use case
| mock.restore(); | ||
| }) | ||
|
|
||
| test('No config creates dir', (t) => { |
There was a problem hiding this comment.
Can you add a test for calling createConfig() with out a directory?
2 participants
First test cases and setup for testing.
Run either
Locally to try
Currently, this tests the postinstall script