feat: use logger instead of console where possible - #14470
feat: use logger instead of console where possible#14470florian-lefebvre wants to merge 3 commits into
Conversation
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
|
Preview deployment ✅ Deployment complete!
|
ArmandPhilippot
left a comment
There was a problem hiding this comment.
Thanks, Florian! Sorry this is only a partial review for now as I have to go. But, I suspect we want a similar change on the transform() API.
My thinking is that onRequest() and transform() remain the main topic. The logger description is maybe "too visible" and can distract from the main purpose of the section. The goal is not to log, even though this is a nice addition. 😄
ArmandPhilippot
left a comment
There was a problem hiding this comment.
Review part 2, see my comment. This is a similar feedback as the one I gave on onRequest(), but as TL/DR: I think it's even trickier than the previous one because of the four distinct parameters.
Not right now, but I’ll try to take the time to think about how we can improve the API references. I think highlighting optional properties is helpful (and this was already on my TODO list)... but maybe we should also have a way to highlight additional arguments. They do not always fit well into the existing description / this requires a more extensive rewrite to include them. 🤔
|
Always happy to find weird cases 😂! I think your suggestions made sense, done them locally |
ArmandPhilippot
left a comment
There was a problem hiding this comment.
I left a suggestion regarding parseURL() because I think there is an issue with the flow. Otherwise, LGTM!
| **Required for local services only; unavailable for external services** | ||
|
|
||
| This hook parses the generated URLs by `getURL()` back into an object with the different properties to be used by `transform` (for on-demand rendering and in dev mode). It is unused during build. | ||
| This hook parses the generated URLs by `getURL()` back into an object with the different properties to be used by `transform` (for on-demand rendering and in dev mode). It is unused during build. This receives three parameters: the URL to parse, the image configuration and, since Astro v7.3.0, a logger. |
There was a problem hiding this comment.
I could be wrong (not an English expert...) but I think "This" after "It" is confusing? And, maybe this requires a bit of reorganization to make the whole thing flow better? Something bothers me with this one.
Maybe:
| This hook parses the generated URLs by `getURL()` back into an object with the different properties to be used by `transform` (for on-demand rendering and in dev mode). It is unused during build. This receives three parameters: the URL to parse, the image configuration and, since Astro v7.3.0, a logger. | |
| This hook parses the generated URLs by `getURL()` back into an object with the different properties to be used by `transform`. This receives three parameters: the URL to parse, the image configuration and, since Astro v7.3.0, a logger. | |
| This hook is used only for on-demand rendering and in development mode. It is unused during build. |
Description (required)
Updates API references
References
7.3.0