Improve config error handling #7852
Conversation
|
github-actions
bot
added
the
pkg: astro
There was a problem hiding this comment.
This file is a rename from load-settings.ts but Git doesn't see it that way.
| try { | ||
| result = await AstroConfigRelativeSchema.parseAsync(userConfig); | ||
| } catch (e) { | ||
| // Improve config zod error messages | ||
| if (e instanceof ZodError) { |
There was a problem hiding this comment.
Errors are now logged more directly.
| // eslint-disable-next-line no-console | ||
| beforeRestart: () => console.clear(), |
There was a problem hiding this comment.
The beforeRestart was only created because on console.clear(), as we don't want to call console.clear in tests (test results gets removed).
Personally I find clearing the console moves and cuts out the terminal a lot that it's better to not do it instead, so I removed it.
There was a problem hiding this comment.
Totally agree, especially if there are scripts/users that rely on stdout/stderr
| experimental: { | ||
| assets: typeof flags.experimentalAssets === 'boolean' ? flags.experimentalAssets : undefined, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
do we have a --experimentalViewTransitions flag?
There was a problem hiding this comment.
I think we don't; I believe we forgot about it. We should probably add it.
| experimental: { | ||
| assets: typeof flags.experimentalAssets === 'boolean' ? flags.experimentalAssets : undefined, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
I think we don't; I believe we forgot about it. We should probably add it.
| // eslint-disable-next-line no-console | ||
| beforeRestart: () => console.clear(), |
There was a problem hiding this comment.
Totally agree, especially if there are scripts/users that rely on stdout/stderr
| }); | ||
| } catch (e) { | ||
| const configPathText = configFile ? colors.bold(configFile) : 'your Astro config'; | ||
| // Config errors should bypass log level as it breaks startup |
There was a problem hiding this comment.
About the comment: Is it because --silent can potentially not log these errors?
There was a problem hiding this comment.
Yeah --silent would otherwise not log that the config failed to load. Try to pass the logging down here is a bit tricky, especially the fact that an Astro config logLevel can also contribute to how logging works.
There was a problem hiding this comment.
Actually there's no logLevel config in the Astro config, got it mixed up. I think it's still probably fine always logging it here as the other code in this file also does that, but I'll revisit this again if it's not ideal.
Changes
dev()public JS API (removebeforeRestartandhandleConfigError)Testing
Existing tests should pass. We don't have a lot of these kind of tests, I also tested the commands manually.
Docs
n/a. internal change, the error messages should still appear unchanged.