feat(config): throw on invalid config instead of using fallback

[Aghassi]

Description

I believe, and I could be wrong, that when the config read (specifically in the case of a .js config) is malformed, syncpack falls back on and uses a default config. The reason that I think this is the wrong solution is because it can lead to scenarios where in syncpack passes when it should instead fail.

Suggested Solution

If syncpack.config.js fails for any reason (like missing dependencies in my case due to uninstalled node_modules), syncpack needs to bubble up that error and exit with code 1 to inform me that the config is malformed. This would have prevent a large headache for me of trying to figure out why syncpack was running but not failing.

Help Needed

Just confirmation that the behavior I described above is intended and if it is ok with being improved

[JamieMason]

Hey @Aghassi, that's right.

1. The config file is read on this line:

   syncpack/src/lib/get-input/get-config.ts

   Line 33 in 13e6c70

   const rcFile = disk.readConfigFileSync(program.configPath);

2. .js config files are read using cosmicconfig on this line:

   syncpack/src/lib/disk.ts

   Line 38 in 13e6c70

   const result = configPath ? client.load(configPath) : client.search();

3. If the .js config file throws or isn't found then an empty object is returned by disk.readConfigFileSync.

4. The rest of that getConfig function then attempts to read each config value and uses it only if it's valid, otherwise using a default if missing or invalid.

Possible Approach

There is currently some undocumented logging which can be enabled like so

SYNCPACK_VERBOSE=true syncpack # list/format/fix-mismatches/etc

I think what I'd want to do is:

1. Add/update logs to make better use of log levels (in descending order of amount: debug, info, warn, error, silent).
2. Set the default log level to error, so that errors start appearing in output for everyone.
3. Expose a logLevel in config so that it can be set to silent if needed, to return to how it works today.
4. Remove SYNCPACK_VERBOSE and make use of the above.

Notes/Questions:

1. Not being able to find a config file is not an error.
2. cosmiconfig finding a config file but it being malformed is an error.
3. Does cosmiconfig let us tell the difference? (I've not checked the docs yet).
4. The typical CLI output should probably not be affected by the logLevel – running syncpack list with --log-level silent and getting no output would be weird.

I like the idea but it's probably a low priority, compared to some of the other features coming up.

Thanks a lot for this and your other issues, they've all helped improve syncpack a lot.

[Aghassi]

My pleasure! This tool has been invaluable in trying to keep things aligned while we scale and dedupe dependencies. Until or unless we have one workspace these things matter.

Per your questions:

1. Not being able to find a config file is not an error.

Agreed, having a default makes sense in this case.

2. cosmiconfig finding a config file but it being malformed is an error.

Agreed here too, because otherwise falling back on the default is confusing

3. Does cosmiconfig let us tell the difference? (I've not checked the docs yet).

Yes loading the config will reject a Promise, see here https://www.npmjs.com/package/cosmiconfig#explorerload

4. The typical CLI output should probably not be affected by the logLevel – running syncpack list with --log-level silent and getting no output would be weird.

To rephrase and I think agree, the result of the CLI should ignore the log level, but internal information around debug statements should show based on log level. If so then yes. Without adding another library you can rely on debug from util https://nodejs.org/api/util.html#utildebugsection. Then just set some env variable based on flag.

[JamieMason]

Released in 10.6.1. Any problems, please let me know.

If you find syncpack useful, please star the project or help us spread the word to other Developers.

[Aghassi]

Solid thanks! I still have to get us to 10.x. I have an issue with rules_js exiting with the incorrect error code and I haven't figured out why yet. Been busy. Will get there though! I'm looking forward to all the improvements :)

[JamieMason]

This version might help with that, I've reverted lint semver groups from erroring when it sees a non semver version, which I think I did in V10.

[Aghassi]

This version might help with that, I've reverted lint semver groups from erroring when it sees a non semver version, which I think I did in V10.

Oh man that actually might be it! Ok I'll upgrade my diff internally and see. Very excited to take in the new features ❤️

[Aghassi]

@JamieMason Just confirming I'm able to take in the latest version without issue. And it has the pnpm fixes too! This is AWESOME. Thank you for all your hard work!

[JamieMason]

You're welcome mate, which pnpm fix was it by the way?

[Aghassi]

@JamieMason workspace:* support, that fix :)