Logging refresh #8833
Logging refresh #8833
Conversation
|
github-actions
bot
added
pkg: integration
|
Having to jump to the browser to see the full error is a huge DX loss in my book, tabbing to the browser is slow and just slows down my feedback loop in addition to requiring a context switch. Rest looks fine, though I'll note that the count being added to the images was an intentional choice requested by the community. |
FredKSchott
force-pushed
the
logging-rewrite
branch
2 times, most recently
from
10bc5cd to
4409514
Compare
|
Why the switch to military time? (I don't mind but feel like users might find that confusing or annoying) |
It should probably just be changed to use whatever the system has configured. Most countries in the world don't use 12-hour formatting. |
|
Suggestion:
|
There was a problem hiding this comment.
I am with @Princesseuh on this one. Forcing the user to switch to the browser to see the whole stacktrace is a considerable loss in terms of DX.
Overall, I like the new logs, but the new APIs should be reworked IMHO, especially considering that we now expose the logger to integrations, too.
| @@ -98,6 +99,27 @@ function padStr(str: string, len: number) { | |||
| return str + spaces; | |||
| } | |||
|
|
|||
| export function getEventPrefix({ level, label }: LogMessage) { | |||
There was a problem hiding this comment.
I can't understand what's this function for, especially in the context of a logger, where we don't have any "event" concept exported externally, and it seems to be only an internal thing.
There was a problem hiding this comment.
This is used by both the console logger and the node logger. The export here is so that both loggers can use it, not meant to signal that it's some public API.
I'll add some JSDoc here to the function to clarify.
| @@ -150,7 +170,7 @@ export async function generatePages(opts: StaticBuildOptions, internals: BuildIn | |||
| } | |||
|
|
|||
| const verb = ssr ? 'prerendering' : 'generating'; | |||
| logger.info(null, `\n${bgGreen(black(` ${verb} static routes `))}`); | |||
| logger.info('SKIP_FORMAT', `\n${bgGreen(black(` ${verb} static routes `))}`); | |||
There was a problem hiding this comment.
Using a label to "skip" the formatting seems fine to me, but not in this way:
- it's not documented anywhere
- it gives a precedent to add more "known labels" to stuff that isn't related to the label, but to the log itself
I would prefer to have this as an alternative:
logger.info("label", "msg", false), where thefalseis to say that it's an unformatted loglogger.unformatted_info("label", "msg")
I prefer 2.. This gives us a way to document what's an "unformatted log"
There was a problem hiding this comment.
I actually agree with you that a more formal API would be great here. I was just intentionally trying to keep any API changes out of scope for this PR to keep focus as much as possible.
Do you have any objection to keeping SKIP_FORMAT in this PR, and then me making a separate PR with a more formal API as a fast-follow?
I'm happy to do either! But I could see us going back-and-forth on that API and this PR is already large enough & far-reaching enough as it is, I wanted to separate the two.
There was a problem hiding this comment.
I'm happy with doing a follow up PR!
There was a problem hiding this comment.
Update: I made some small changes here to type this in TS, to keep our label values fairly standardized, prevent typos, etc.
This API feels a bit more formal now, so possibly worth another look if this can be the final API? Otherwise, still happy to do that follow-up PR but tbh I kind of like this.
There was a problem hiding this comment.
I definitely like this better than everything being undocumented! I personally dislike our label system, so I'd be happy with a follow-up to reformat, remove, or refactor the API for them.
The current API feels odd because the very first argument is optional (and we're now using a special string to opt-out instead of undefined or null). It would be better if the logger supported an options bag with a label as the last argument.
This is a misunderstanding of the change / this is not an expected outcome. The console includes all relevant stack-trace information from your own codebase. The information that is stripped out is from internal stack trace information from within your node_modules directory. If this entire stack trace happened within your own codebase, it would show everything. You can see the logic here: https://github.com/withastro/astro/pull/8833/files#diff-3fe94cb3205692ae94a94644f96a44ee7a05492f2ff3b07c4b6d6bb7ad3dc142R258-R274 Example: Error: Something happened!
at fooFunction in /src/utils.js:1:2
at barFunction in /src/db.js:1:2
at /src/components/Header.astro:4:7
- at AstroComponentInstance.Header [as factory] (/node_modules/astro/dist/runtime/server/astro-component.js:18:12)
- at AstroComponentInstance.init (/node_modules/astro/dist/runtime/server/render/astro/instance.js:26:29)
- at AstroComponentInstance.render (/node_modules/astro/dist/runtime/server/render/astro/instance.js:31:18)
- at Object.render (/node_modules/astro/dist/runtime/server/render/component.js:330:22)
- at Module.renderChild (/node_modules/astro/dist/runtime/server/render/any.js:29:17)
+ See full error details in the browser.A reminder that this is what many of our users see in the terminal when an error happens today:
|
Mainly for conciseness,
Yea, as you're kind of capturing it perfectly here. It isn't very important (a non-goal) that the developer sees "16:11:22" and says "ah, ha, this happened at 4:11pm!". The goal of the log timestamp is more important as a way to understand the difference in relative time between logs, and between the latest log and however far back you're scrolling. Both of these are important tools when debugging something in the terminal output. To illustrate this: this time could be in UTC or local time, and it would be equally helpful. |
FredKSchott
force-pushed
the
logging-rewrite
branch
from
4409514 to
e6d981e
Compare
I mean that it removes all of the formatting in favour of a technical information. In Astro, I never look at the stacktrace (including it's message) because we have a humanly formatted version at the top in the CLI. This is even more so the case for Astro errors, where we have a hint that is now removed. This also removed the The shortened stack trace is great though. I'd make sure it works in all situations though because Astro stack traces are a bit wonky and parsing stacktraces is hard |
Another option here would be to keep the current "detailed" formatting (with hint, cause, etc) but add this new stack trace simplification logic. That would actually still address my main goal here of solving the issue where smaller terminals only show the useless stacktrace noise on any error. @Princesseuh @ematipico wdyt? |
I'd agree with this! I'd love to see this logic in the browser as well actually, not necessarily fully removing the unrelated entry, but graying them out perhaps. (Phoenix has this, and actually allows you to click on every line to see the file in question, it's pretty neat) |
|
I took another stab at this but still couldn't make it work in a smaller or medium-sized terminal. So I took a different approach and just added the missing information to the new concise format that I had been working on. This new format now displays Now, the only pieces of information that are intentionally not included in the new format are the code frame, and the un-abridged stack trace. Some screenshots of the new format: 
|
| @@ -80,7 +93,6 @@ export const StaticClientAddressNotAvailable = { | |||
| title: '`Astro.clientAddress` is not available in static mode.', | |||
| message: | |||
| "`Astro.clientAddress` is only available when using `output: 'server'` or `output: 'hybrid'`. Update your Astro config if you need SSR features.", | |||
| hint: 'See https://docs.astro.build/en/guides/server-side-rendering/#enabling-ssr-in-your-project for more information on how to enable SSR.', | |||
There was a problem hiding this comment.
@Princesseuh The link to the docs error reference URL is now more prominent in the new error log format. Therefore, I updated all hints to remove any that were just calling out additional external documentation URLs. These were almost always already included on our own docs page, so removing them from the CLI reduced noise/distraction ("which URL do I click?")
Added a note to the README to clarify for the future that hint should only be used for suggesting (err... "hinting") a specific solution or fix to the user.
✅ "Does the file exist?"
❌ "See https://docs.astro.build/en/guides/some-docs-that-may-or-may-not-help-you-solve-this for more information"
I think this makes sense given the new error log format (see screenshots in my last comment) but wanted to make sure you saw this and get your thoughts, if any!
There was a problem hiding this comment.
From @Princesseuh (threading this here to keep the convo going in one place):
In general, the entire purpose of the hints wasn't necessarily to be short, right. They're supposed to get users to potential solutions as soon as possible, so they should be concise (much like error messages), but not omit information that they can in fact provide.
Removing links from them to link to the error reference in addition of potentially delaying finding a solution, requires the user to do multiple context switches when we can link to docs page that explain the domain directly.
I don't personally see the value of reducing the length of errors that much. If I get an error, I'd rather the terminal be spammed with all relevant information than be pretty looking (and I'd much rather resize my terminal than visit multiple webpages)
We could argue that this new format makes it easier to ingest information, but I don't think that's true because of things being all together vs in different places. It just looks, subjectively, prettier.
(Edit, oops this was meant to be a reply to the comment, but for some reason GitHub Mobile put it top level)
There was a problem hiding this comment.
In general, the entire purpose of the hints wasn't necessarily to be short, right. They're supposed to get users to potential solutions as soon as possible, so they should be concise (much like error messages), but not omit information that they can in fact provide.
That's a great callout. Yes, this PR now explicitly changes the purpose of hint to point you to the solution but not educate you. That job is left to the error reference URL which is always included in the log, now more prominently than before.
As a reminder, this is what a new error log looks like, with a hint + error reference URL included and clearly visible.
The reasoning is that having docs and links in the hint is actually less effective for all users.
- For users who don't need education (ex: they already know what the
altattribute is) it's more verbose and adds mental overhead to parse the message for the relevant info you need to solve. - For the user who does need education, 2+ links is less effective than 1, very clear "go here to learn more" URL. Putting on my new user hat: "wait, this error reference is a URL to learn more but then this hint also includes 1+ different URLs that might help me, which one should i be clicking?"
We should trust that our error reference pages in docs are the best place to go to learn what the error is and how to solve it. If they aren't, then right solution should be to improve them vs. working around them by adding different additional URLs to the error log.
There was a problem hiding this comment.
I see your point, but I (respectfully) don't agree.
Of course, it's on a error to error basis right, I can probably agree to remove some of the ones that are just links, but I don't agree with generally always shortening the hints and removing the information.
The alt one is the most telling to me: I do not mean to exaggerate when I say I find it frustrating to the read with the information removed. Without removing the text completely, just a The alt property is important for the purpose of accessibility. Use alt="" if image is not a key part of page content. would massively improve it imo if we don't want to potentially overwhelm the user, but still conveys why it's needed without opening a web page.
For context, I mostly based the work on errors on Elm and Rust, both projects widely acknowledged for their empowering and instructive errors and hints. Rust mostly achieve it through showing your own code, which we can't do a lot in Astro, whereas Elm has quite literally full page errors crafting an entire story in the first-person around the error (it's a lot).
To be honest, I kinda find all the information cramped like this to be very hard to read! Modern error reporting is now quite formatted, like this example from Miette:
or Rust:
There was a problem hiding this comment.
I wouldn't over-index on any one change made here, as I mentioned elsewhere I likely removed more than neccesary and am happy to workshop any of these individually (see specific improvements below).
If you'd like to keep using the "alt" example as representative of the rest of these changes, I'm happy to. Here's what I'd suggest now based on your first pass of feedback:
vs. what we have today (reminder: in a window that I had to expand from VSCode's default size):
The two pieces of information that have been removed:
- why "alt" is required: The "why" doesn't materially change the error, how to solve, or the fact that alt is required. It is definitely interesting context, and in the new screenshot the user is still free to click the link for more information to learn why alt exists. But this is unnecessary background context to show to every user on every of this error, and slows down debugging by roughly doubling the amount of text in the error.
- a link to the MDN docs on the "alt" attribute: Putting aside that the
#attr-altURL hash is now broken and the impossibility of maintaining external links that we don't control, this information (and the direct link to MDN) are both already contained at the docs.astro.build URL. Showing two URLs makes it unclear where the user should go to learn more about the error.
I stand by both of these changes as objective improvements, simplifying the error for faster parsing by the user without losing important information.
There was a problem hiding this comment.
Also, please know that none of this is meant as a criticism of your work and I appreciate the discussion! Subjective changes like "make error pretty" are hard to talk through so I'm trying my best to distill to objective truths about what we do well today and what we could do better.
The fact that we even have hints and hosted error references in our docs at all is huge! Ironically, my intention here is to highlight both of these more than we do today. For ex, if the docs URL isn't the decisive resource to explain the error and the context behind it, we should improve the content on that page vs. pointing you somewhere else.
There was a problem hiding this comment.
This suggestion is enough for me for this particular error! For this error in particular, there's multiple reasons why it over-explained the why:
- The
altattribute is not required onimg, and our docs give a pretty clear "img to Image" story (or at least, used to when this message was made) - The
altattribute by itself doesn't mean anything, it's easy to guess why asrcattribute is required, but if you don't know whataltis, it's frustrating to be told it's required with no direct context, and, as mentioned below, needing to visit a web page to be explained why is not a good experience.
The link might've been unneeded, and the text could've made been more concise, but it was intentional for it to explain the purpose in a quite detailed fashion.
I generally don't agree on relying on the docs website to explain the error. To me it was a layer system where every part could alone help you to fix the problem depending on what you know / want to know and the docs link is the last chance (or well, second to last with the "see more" links)
In my mind, if an error was solved without going to the docs reference, that means it was a good error. Even without mentioning the obvious "the docs doesn't work offline", "the docs can take a long time to load" etc etc, the last thing I want to do when I have an error is have to read a web page to understand it because the message is lacking information that it could very easily have (not to mention the ability of error messages / hints to have contextual information, unlike the docs)
Another thing too, but that's more in writing and probably can be achieved even with the goal of having less information in the error is that I purposely tried to write (emphasis on tried, because I definitely didn't rewrite every error message) messages using a human voice, not skipping words for brevity etc. Some of Astro's strengths, in my mind, are being straightforward, honest and human, which I feel is better represented with human-voice messages, even if it cost some brevity (Elm has a similar philosophy, with its first person messages)
There was a problem hiding this comment.
Subjectively I'd like to have as much information in the terminal as possible. I don't want to have to flip over to a webpage to understand what I did wrong. But that also depends on how well I know what i'm doing, I take the point about conciseness being better if you already are aware of the problem. I'm not sure there is any one correct answer here.
What's unclear to me from this PR is if the motivation is about the error text or if its about fitting the message within a vscode window. If it were about a space concern there could be a short and long hint that gets displayed depending on the size of the terminal. But it's not clear to me that that's the true motivation, only that I've seen it brought up several times in this PR as a partial motivation.
FredKSchott
force-pushed
the
logging-rewrite
branch
from
de24d90 to
f1ff336
Compare
| * - [Configuring a UI framework integration](https://docs.astro.build/en/core-concepts/framework-components/) | ||
| * - [List of all official UI framework integrations](https://docs.astro.build/en/guides/integrations-guide/#official-integrations) |
There was a problem hiding this comment.
Those links (and all the other @see when possible) kinda purposely matched the headers they're linking to, so that there's a mental link between where you clicked vs where you landed. I believe the idea is reducing the cognitive shift needed after landing on the page
(The second link is okay, since it just links to a list and there's no text to read)
There was a problem hiding this comment.
I'm okay with either! I'll revert if that's your preference.
| @@ -493,7 +484,7 @@ export const ImageMissingAlt = { | |||
| name: 'ImageMissingAlt', | |||
| title: 'Missing alt property.', | |||
| message: 'The alt property is required.', | |||
| hint: "The `alt` property is important for the purpose of accessibility, without it users using screen readers or other assistive technologies won't be able to understand what your image is supposed to represent. See https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img#attr-alt for more information.", | |||
| hint: 'Use alt="" if image is not a key part of page content.', | |||
There was a problem hiding this comment.
This lost a ton of information. This should be written from the perspective of not knowing what an alt tag is and why it's required, because unlike other similar errors, it is not required for a technical reason, so users don't see the purpose without a more detailed description. It was one of our users favorite errors for how descriptive it was...
This edit ends up being frustrating to read due to the lack of info.
There was a problem hiding this comment.
See longer response above: #8833 (comment)
I'm happy to work with you to improve any of these new hints, if we can keep my new breakdown of responsibility as "hints just hint, error reference URLs educate". I'm sure I err'd on the side of removing vs. keeping, so it makes total sense that some things should be added back.
+1 on adding back a note on why it's required. Let me give this a rewrite and then LMK what you think.
There was a problem hiding this comment.
if we can keep my new breakdown of responsibility as "hints just hint, error reference URLs educate"
Why is this the new breakdown of responsibility? What is the motivation?
| @@ -810,14 +803,14 @@ export const LocalImageUsedWrongly = { | |||
| name: 'LocalImageUsedWrongly', | |||
| title: 'Local images must be imported.', | |||
| message: (imageFilePath: string) => | |||
| `\`Image\`'s and \`getImage\`'s \`src\` parameter must be an imported image or an URL, it cannot be a string filepath. Received \`${imageFilePath}\`.`, | |||
| hint: 'If you want to use an image from your `src` folder, you need to either import it or if the image is coming from a content collection, use the [image() schema helper](https://docs.astro.build/en/guides/images/#images-in-content-collections) See https://docs.astro.build/en/guides/images/#src-required for more information on the `src` property.', | |||
There was a problem hiding this comment.
This hint was there because this is an insanely common mistake due to how unintuitive it is to not being able to pass image paths.
We've had a lot of support threads about it, which in my experience, being in the Discord all the time, decreased after I purposely added more information to this message.
There was a problem hiding this comment.
See longer response above: #8833 (comment)
I'm happy to work with you to improve any of these new hints, if we can keep my new breakdown of responsibility as "hints just hint, error reference URLs educate". I'm sure I err'd on the side of removing vs. keeping, so it makes total sense that some things should be added back.
Let me give this a rewrite and then LMK what you think.
There was a problem hiding this comment.
I have to agree with Erika's original wording here, or something closer to that. This is not only how we phrase it in docs (you have to import the image first to be able to use it) but also what we keep saying over and over in support. And, that phrase does seem to get the idea across to people. I think the new suggestion loses that. ("use the ESM import keyword" seems less helpful than "import the image")?
github-actions
bot
added
the
pr: docs
|
This have not dug into the code yet to give a proper review, but I just want to say that I absolutely love this effort! CLI design and consistency is my jam. It's incredible what an impact stuff like this can have on the perceived polish of a framework. Great job! |
|
I also haven't read through it yet either, but to chime in on the stack trace discussion; one problem with trimming out framework code from a stack trace is that frameworks have bugs too! So trimming that away makes it harder for users to report bugs and for us to fix them. One compromise might be to put the full stacktrace until a |
| message: (imageFilePath: string) => | ||
| `\`Image\`'s and \`getImage\`'s \`src\` parameter must be an imported image or a URL, it cannot be a string filepath. Received \`${imageFilePath}\`.`, | ||
| `Image "src" value not supported. Local images must be imported with the ESM "import" keyword or the "image()" schema helper in a content collection. Received "${imageFilePath}".`, |
There was a problem hiding this comment.
I purposely use backticks because they show with special highlighting in the browser (and was honestly hoping to add special highlighting in the CLI too one day)
There was a problem hiding this comment.
I know docs wasn't tagged in yet, and I'm not joining in the hint/docs conversation. But left some comments on things that I noticed in the error/warn messages etc, plus in the definition of a hint itself! However this all shakes out, happy to see all the attention paid to our messages!
| - A `hint` can be used for any additional info that might help the user. (ex: a link to the documentation, or a common cause) | ||
| - A `hint` can be used to display additional information that might help the user resolve their issue. | ||
| - A good hint points the user towards a possible solution. "Did you mean X?" is an example of a good hint. | ||
| - Don't use a hint to link to docs, or include additional documentation about the error. Instead, make sure that good documentation is included in the `@docs` JSDoc, which the user will automatically see a link to in their formatted error logging. |
There was a problem hiding this comment.
| - Don't use a hint to link to docs, or include additional documentation about the error. Instead, make sure that good documentation is included in the `@docs` JSDoc, which the user will automatically see a link to in their formatted error logging. | |
| - Don't use a `hint` to link to docs, or include additional documentation about the error. Instead, make sure that good documentation is included in the `@docs` JSDoc. This will automatically be provided in the formatted error logging and does not need to be included as a `hint`. |
|
|
||
| Hint: | ||
|
|
||
| - A `hint` can be used for any additional info that might help the user. (ex: a link to the documentation, or a common cause) | ||
| - A `hint` can be used to display additional information that might help the user resolve their issue. |
There was a problem hiding this comment.
| - A `hint` can be used to display additional information that might help the user resolve their issue. | |
| - A `hint` can be used to display a suggestion or clarifying question that might help the user resolve their issue. |
Might help differentiate from e.g. additional docs content if a hint is defined as a suggestion rather than just additional info.
| '⚠️ Serving with vite.server.fs.strict: false. Note that all files on your machine will be accessible to anyone on your network!' | ||
| ); | ||
| const title = yellow('▶ ' + `${bold('vite.server.fs.strict')} has been disabled!`); | ||
| const subtitle = ` Files on your machine are likely accessible on your network.`; |
There was a problem hiding this comment.
| const subtitle = ` Files on your machine are likely accessible on your network.`; | |
| const subtitle = ` Files on your machine could be accessible over your network.`; |
Maybe a slight tweak that strikes the balance between the current and the proposed? While still non-threatening, I think this might be a slightly clearer warning that conveys what the original meant to.
There was a problem hiding this comment.
I'm not sure how useful the could, likely etc nuance is, fs.strict does give access to every file on your network. Sure, maybe, perhaps, there's another software running preventing this, but by itself, that's what it does!
There was a problem hiding this comment.
I just felt like THIS COULD HAPPEN is more interpreted as a warning phrase, even if, it totally does happen. The original seemed like a pretty strong warning, and the new doesn't feel like it would jump out to the reader as much. Just a thought!
| } | ||
| logger.warn( | ||
| null, | ||
| `Unsupported file type ${bold( |
There was a problem hiding this comment.
| `Unsupported file type ${bold( | |
| `Unsupported page file type ${bold( |
If still accurate, I do like mentioning that the file type is unsupported for use as a page.
| `${adapterName}`, | ||
| `The feature ${featureName} is not supported by the adapter ${adapterName}.` | ||
| 'config', | ||
| `The feature ${featureName} is not supported (used by ${adapterName}).` |
There was a problem hiding this comment.
I'm not sure what the "used by" adapeter in parenthesis is supposed to convey here? I would be confused to see this message, for example:
The feature confetti is not supported (used by Cloudflare).
Same goes for the ones below.
| @@ -810,14 +803,14 @@ export const LocalImageUsedWrongly = { | |||
| name: 'LocalImageUsedWrongly', | |||
| title: 'Local images must be imported.', | |||
| message: (imageFilePath: string) => | |||
| `\`Image\`'s and \`getImage\`'s \`src\` parameter must be an imported image or an URL, it cannot be a string filepath. Received \`${imageFilePath}\`.`, | |||
| hint: 'If you want to use an image from your `src` folder, you need to either import it or if the image is coming from a content collection, use the [image() schema helper](https://docs.astro.build/en/guides/images/#images-in-content-collections) See https://docs.astro.build/en/guides/images/#src-required for more information on the `src` property.', | |||
There was a problem hiding this comment.
I have to agree with Erika's original wording here, or something closer to that. This is not only how we phrase it in docs (you have to import the image first to be able to use it) but also what we keep saying over and over in support. And, that phrase does seem to get the idea across to people. I think the new suggestion loses that. ("use the ESM import keyword" seems less helpful than "import the image")?
| @@ -890,8 +884,7 @@ export const MissingSharp = { | |||
| name: 'MissingSharp', | |||
| title: 'Could not find Sharp.', | |||
| message: | |||
| 'Could not find Sharp. Please install Sharp (`sharp`) manually into your project or migrate to another image service.', | |||
| hint: "See Sharp's installation instructions for more information: https://sharp.pixelplumbing.com/install. If you are not relying on `astro:assets` to optimize, transform, or process any images, you can configure a passthrough image service instead of installing Sharp. See https://docs.astro.build/en/reference/errors/missing-sharp for more information.\n\nSee https://docs.astro.build/en/guides/images/#default-image-service for more information on how to migrate to another image service.", | |||
| 'Could not find npm dependency package "sharp". Please install Sharp manually into your project or migrate to another image service.', | |||
There was a problem hiding this comment.
This loses the helpful context about a passthrough/noop service being available, which is not fully conveyed by "install or migrate to another service." (I wouldn't get from this that a non-service would exist.) Could still be done briefly, maybe something like:
| 'Could not find npm dependency package "sharp". Please install Sharp manually into your project or migrate to another image service.', | |
| 'Could not find npm dependency package "sharp". Please install Sharp manually into your project or choose another image service configuration.', |
| @@ -921,7 +914,7 @@ export const FailedToLoadModuleSSR = { | |||
| name: 'FailedToLoadModuleSSR', | |||
| title: 'Could not import file.', | |||
| message: (importName: string) => `Could not import \`${importName}\`.`, | |||
| hint: 'This is often caused by a typo in the import path. Please make sure the file exists.', | |||
| hint: 'Does the file exist?', | |||
There was a problem hiding this comment.
Makes my snark meter go off the charts...
| message: (legacyConfigKey: string) => `Legacy configuration detected: \`${legacyConfigKey}\`.`, | ||
| hint: 'Please update your configuration to the new format.\nSee https://astro.build/config for more information.', | ||
| message: (legacyConfigKey: string) => | ||
| `Legacy configuration detected: \`${legacyConfigKey}\`. update your configuration to the new format.`, |
There was a problem hiding this comment.
| `Legacy configuration detected: \`${legacyConfigKey}\`. update your configuration to the new format.`, | |
| `Legacy configuration detected: \`${legacyConfigKey}\`. Update your configuration to the current supported format.`, |
Just while I'm here... I don't like referring to current, existing things as "new" because that doesn't age well. At some point, this isn't "new", it's just the way it is. This is more future-proof.
| @@ -1270,7 +1258,7 @@ export const UnsupportedConfigTransformError = { | |||
| title: 'Unsupported transform in content config.', | |||
| message: (parseError: string) => | |||
| `\`transform()\` functions in your content config must return valid JSON, or data types compatible with the devalue library (including Dates, Maps, and Sets).\nFull error: ${parseError}`, | |||
| hint: 'See the devalue library for all supported types: https://github.com/rich-harris/devalue', | |||
| hint: 'See all supported types: https://github.com/rich-harris/devalue', | |||
There was a problem hiding this comment.
Slight personal preference towards the old, because I do like pointing out that this is an external (not Astro) link. I like telling people where they are going, and most of our error messages take us to our own documentation.
| return stackLines.join('\n'); | ||
| } | ||
| } | ||
| // If the error occurred inside of a dependency, grab the entire stack. |
There was a problem hiding this comment.
Good comment, am I correct that if the error occurs within Astro that it will show the full stack?
|
Code-wise looks pretty good, appreciate the code comments especially around the stack trace parsing. Feature wise, a couple of points of feedback:
|
| - A `hint` can be used for any additional info that might help the user. (ex: a link to the documentation, or a common cause) | ||
| - A `hint` can be used to display additional information that might help the user resolve their issue. | ||
| - A good hint points the user towards a possible solution. "Did you mean X?" is an example of a good hint. | ||
| - Don't use a hint to link to docs, or include additional documentation about the error. Instead, make sure that good documentation is included in the `@docs` JSDoc, which the user will automatically see a link to in their formatted error logging. |
There was a problem hiding this comment.
is this change because there is double links otherwise? This is not removing all links, is it?
|
Thanks again for the reviews, everyone. I haven't forgotten this! I caught up with @matthewp about this PR in person the other week and we decided that the best path forward would be to pull the error changes out of this PR, since they seem to be the most controversial. I'll close this PR and re-open a new one with these updates. |
Changes
[label]information from most logs(x2),(x3), etc. effect in the dev serverVideos
logging-rewrite-old.mp4
logging-rewrite-new.mp4
Screenshots
Example:
(xN)Formatting WeirdnessRewriting the CLI output would cause weird formatting issues sometimes. In this example, resizing the CLI window did it.
Example: Consistent Formatting
Before
After
Better CLI Error Formatting
Before
Too big to display in smaller/medium terminal windows. If you scrolled, or expanded the terminal window to contain the entire error:
After
Build Output
Mainly stylistic, colors changed intentionally.
Before
After
Testing
Docs