Skip to content
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

Possible Unintended Side Effects of: always look at .config.{yml,yaml,json,js,cjs} #296

Closed
Jason3S opened this issue Mar 16, 2023 · 7 comments

Comments

@Jason3S
Copy link

Jason3S commented Mar 16, 2023

8.1.0

  • Added: always look at .config.{yml,yaml,json,js,cjs} file to configure cosmiconfig itself, and look for tool configuration in it using packageProp (similar to package.json)
  • For more info on this, look at the end user configuration section of the README

Is there a way to turn this off? I'm hunting down a crashing issue streetsidesoftware/vscode-spell-checker#2583 that is most likely caused by a user's .config.* changing the .json parser.

@Jason3S Jason3S changed the title Possible: Unintended Side Effects of: _always look at .config.{yml,yaml,json,js,cjs}_ Possible Unintended Side Effects of: always look at .config.{yml,yaml,json,js,cjs} Mar 16, 2023
@d-fischer
Copy link
Member

d-fischer commented Mar 16, 2023

You can explicitly not overwrite loaders in that config file (as of now):

cosmiconfig/src/index.ts

Lines 125 to 127 in dacce13

if (config.loaders) {
throw new Error('Can not specify loaders in meta config file');
}

But if it were possible, it would be the user's own fault, to be honest.

This might be a bug relating to the loading though. Will investigate as soon as your user has answered your question in your issue so I have a proper lead on this.

@Jason3S
Copy link
Author

Jason3S commented Mar 17, 2023

@d-fischer,

He has confirmed that there is a .config.js: streetsidesoftware/vscode-spell-checker#2583 (comment)

The issue is right there in the code, options is lost if there is a metaConfig:

cosmiconfig/src/index.ts

Lines 119 to 138 in dacce13

if (!metaConfig) {
return normalizeOptions(moduleName, options);
}
const config = metaConfig.config ?? {};
if (config.loaders) {
throw new Error('Can not specify loaders in meta config file');
}
if (config.searchPlaces) {
config.searchPlaces = replaceMetaPlaceholders(
config.searchPlaces,
moduleName,
);
}
config.metaConfigFilePath = metaConfig.filepath;
return normalizeOptions(moduleName, config, false);

@d-fischer
Copy link
Member

d-fischer commented Mar 17, 2023

Welp, that's obviously a mistake, it should merge with the original config. Will fix ASAP. Thanks for your investigation.

@Jason3S
Copy link
Author

Jason3S commented Mar 17, 2023

@d-fischer,

Does it make sense to "merger" the searchPlaces instead of replacing them? I can understand if a User wants to add their preferred places, but replacing the ones specified by the cli-tool can easily cause things to break.

For example, cspell has multiple places for a configuration file. It has a defined search pattern. Sub folders and parent folders might both have config files. If the .config.js at the root is there to clean up the root, it would mess up the sub folders.

Case in point, if someone adds a .config.js to adjust the location of prettier, they could break cspell or eslint because a "single" config is applied to ALL tools that use Cosmiconfig.

@d-fischer
Copy link
Member

d-fischer commented Mar 17, 2023

The idea of this "meta config" concept was always to put the end user in a position of power over where their config is. I can maybe see some way to notify the user via a console warning when an empty configuration is returned while a searchPlaces override is in place. This would probably include a way to silence said warning, too.

I don't like the idea of merging the places by default. Maybe there could be an option to enable merging? Maybe even just for specified libraries? I don't know what would be best yet, but these ideas should probably be refined within a separate issue.

@d-fischer
Copy link
Member

Fixed in 8.1.1. Again, thanks for your efforts!

@Jason3S
Copy link
Author

Jason3S commented Mar 17, 2023

@d-fischer,

Thank you for fixing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants