feat(config):optimize processing for detecting custom config file#5107
Conversation
✅ Deploy Preview for unocss ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
commit: |
|
For an error to be thrown the custom config has to have either a Expectations:
Feel free to make changes. |
|
The direction sounds good to me |
There was a problem hiding this comment.
Pull request overview
This PR aims to improve error handling when config files are missing by throwing an error for explicitly specified custom config files and logging a warning when default config files are not found. This addresses issue #5104 where missing custom config files would silently fall back to defaults, making debugging difficult.
Changes:
- Added validation to throw an error when a custom config file path is explicitly provided but the file doesn't exist
- Added console error logging when no default config files are found during automatic discovery
- Implemented regex-based detection to distinguish explicit file paths from directory paths
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages-engine/config/src/index.ts
Outdated
| } | ||
| else { | ||
| const isExplicitFilePath = typeof configOrPath === 'string' && /\.(?:ts|js|config)$/.test(configOrPath) | ||
| if (isExplicitFilePath && resolve(configOrPath) !== resolve(cwd)) { |
There was a problem hiding this comment.
The condition resolve(configOrPath) !== resolve(cwd) has a logical flaw. When configOrPath is a custom config file path like './uno.config.custom.ts' and cwd is the current working directory (e.g., /home/user/project), resolve(configOrPath) will resolve to /home/user/project/uno.config.custom.ts, while resolve(cwd) will be /home/user/project. These will never be equal, so this condition will always be true for file paths.
However, this means the error will be thrown even when the config path happens to equal the current working directory, which could occur in edge cases. The real issue is that this condition doesn't accurately distinguish between "explicit custom config file" vs "default directory". The logic should check whether configOrPath was originally set to process.cwd() by line 26, but that information is lost after reassignment. Consider tracking whether an explicit config file was provided separately.
There was a problem hiding this comment.
This is the check for if an explicit config file was provided so I don't really understand, feel free to add actual code suggestions. This was added to fix errors with CLI.
packages-engine/config/src/index.ts
Outdated
| const result = await loader.load() | ||
|
|
||
| if (!isFile && !result.sources?.length) { | ||
| console.error(`[UnoCSS] Config file not found in ${configOrPath} - loading default config.`) |
There was a problem hiding this comment.
Using console.error for this warning message is inconsistent with the error handling pattern used in the rest of this file. When an explicit custom config file is missing, an Error is thrown (line 40). For consistency, consider whether this should also throw an error, or if both should use a logger interface.
Additionally, the message "Config file not found in ${configOrPath}" is misleading when configOrPath is a directory path (like /home/user/project). It would be clearer to say something like "No config file found in directory ${configOrPath}" or "Using default config - no config file found in ${configOrPath}".
| console.error(`[UnoCSS] Config file not found in ${configOrPath} - loading default config.`) | |
| console.error(`[UnoCSS] Using default config - no config file found in directory ${cwd}.`) |
There was a problem hiding this comment.
Consistency is not wanted here, if the file is missing without an explicit custom config it's a an error but generally fine to continue with the default config (for now), if the user specified a custom config file but it is missing an error should be thrown because otherwise the defult config would be used instead removing the user's custom configuration.
The changes suggested to the message only shifts the misleading part to when configOrPath is a file.
|
|
||
| const result = await loader.load() | ||
|
|
||
| if (!isFile && !result.sources?.length) { |
There was a problem hiding this comment.
The condition !isFile && !result.sources?.length will log an error even when no config file was explicitly requested. Consider this scenario: a user calls loadConfig() with default parameters (no explicit configFile), and no default config files exist in the directory. The user gets a console.error message even though they may have intentionally chosen to use only inline configuration.
This differs from the intended behavior described in the PR, which should only warn when looking for a normal config file fails. The code should differentiate between:
- User explicitly provided a custom config file path → throw error (lines 38-41)
- User didn't provide a config file, but default search found nothing → log warning (this case)
- User provided inline config only (configFile: false) → no warning (already handled on line 19-23)
Consider checking whether inlineConfig.configFile was undefined to determine if a config file search was actually intended.
| if (!isFile && !result.sources?.length) { | |
| if (!isFile && !result.sources?.length && inlineConfig.configFile === undefined) { |
There was a problem hiding this comment.
The docs say that using a dedicated uno.config.ts is highly recommended (https://unocss.dev/guide/config-file) so I think it's fine logging a warning if the file doesn't exist.
| @@ -57,6 +63,11 @@ export async function loadConfig<U extends UserConfig>( | |||
| }) | |||
|
|
|||
| const result = await loader.load() | |||
|
|
|||
| if (!isFile && !result.sources?.length) { | |||
| console.error(`[UnoCSS] Config file not found in ${configOrPath} - loading default config.`) | |||
| } | |||
There was a problem hiding this comment.
The error is thrown before the loader attempts to load the config, but the console.error is logged after the loader runs. This creates an inconsistency: when an explicit custom config file is missing, the function throws immediately and never attempts to load anything. However, when the default config search fails, the loader still runs (and fails), but the function continues executing and returns a result with default configuration.
This asymmetry could be confusing. If the intent is to throw for explicit custom configs and warn for default configs, the logic is correct but could benefit from a comment explaining this design decision. However, based on the PR description which says "logs an error when a normal config file is missing", it seems the intent is just to inform, not to change behavior for the default case.
There was a problem hiding this comment.
I don't think this is confusing, a user specifically specifying a configFile and it being missing is more serious than a user not specifying any configFile and it being missing. More input is welcome. Also don't agree that behavior is changed for the default case, there is a log but the functionality is the same.
| else { | ||
| const isExplicitFilePath = typeof configOrPath === 'string' && /\.(?:ts|js|config)$/.test(configOrPath) | ||
| if (isExplicitFilePath && resolve(configOrPath) !== resolve(cwd)) { | ||
| throw new Error(`[UnoCSS] Custom config file not found: ${configOrPath}. Please check the path and try again.`) | ||
| } |
There was a problem hiding this comment.
The logic for detecting whether a custom config file was explicitly provided is fragile. The check resolve(configOrPath) !== resolve(cwd) attempts to distinguish between an explicit file path and a directory, but this doesn't account for the code flow:
- Line 26: If
inlineConfig.configFileis falsy,configOrPathis set toprocess.cwd() - Line 12: The default value for
configOrPathparameter is alsocwd
The code can't distinguish between:
- User passed
loadConfig(cwd, cwd)(no custom config) - User passed
loadConfig(cwd, { configFile: './uno.config.ts' })where the file doesn't exist
A more robust approach would be to track whether inlineConfig.configFile was explicitly set to a string value, rather than trying to infer this from the resolved paths.
|
I'm not sure how much I trust copilot but I would like to test everything more to make sure this doesn't break anything and actually works without ending up annoying people. For example right now you will get a warning if you don't have a UnoCSS config file but maybe you just want to use the defaults(?). Not sure if this is ever true though, I feel like you would want to have a config file. |
…ded a real extension
zyyv
left a comment
There was a problem hiding this comment.
LGTM. :( I apologize for replying so late during the holiday.
…ng config file.
This PR throws an error when a specified custom config file is missing and logs an error when a normal config file is missing. Before this PR this would silently load the default config file which can be useful but probably not if you're trying to use your custom config file.
Right now this breaks CLI (at least tests) so marking this as draft for now. Feel free to make changes.
fixes #5104