-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Clarify and reduce Cloudflare adapter integration logs noise #13817
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
Conversation
🦋 Changeset detectedLatest commit: 3191a96 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| if (config.service.entrypoint === 'astro/assets/services/sharp') { | ||
| logger.warn( | ||
| `The current configuration does not support image optimization. To allow your project to build with the original, unoptimized images, the image service has been automatically switched to the 'noop' option. See https://docs.astro.build/en/reference/configuration-reference/#imageservice`, | ||
| `The current configuration does not support image optimization. To allow your project to build with the original, unoptimized images, the image service has been automatically switched to the 'passthrough' option. See https://docs.astro.build/en/reference/configuration-reference/#imageservice`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit of an extra, the option name itself is not noop (although yes, it's a non-operational service). Mostly to make things consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logged messages look good to me, Yan! One small suggestion below to see if we can make one a little easier to read. Approving for docs/messages!
| ); | ||
| logger.info( | ||
| `If you see the error "Invalid binding \`${bindingName}\`" in your build output, you need to add the binding to your wrangler config file.`, | ||
| `Enabling sessions with Cloudflare KV for production with the "${bindingName}" KV binding.`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is misleading because in dev it's not using CLoudflare KV. I'd move this inside the conditional block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does say it's Cloudflare KV for production, so I wouldn't think that would apply in dev if I were reading that.
I am not against moving this into the conditional, though! I was thinking, and I guess most people wanting to use Sessions will either look into the docs and configure manually or see the Cloudflare adapter docs and just use the default config, so being reminded of this in dev might not be necessary.
Co-authored-by: Sarah Rainsberger <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer if core actually allows the adapter to control when messages will be logged instead of using a "hacky" workaround, but it works for me.
|
From this PR it's clear that the supporting feature needs to be more flexible and intelligent, so I wonder:
|
|
@ematipico IIRC in the past I was told we don't want to change the logic in core, and the adapter should find a way to work-around it, but it that changed, we can track the idea and try to open PRs. |
|
I agree with @ematipico. Let's fix this in core rather than hacking around it. |
|
I made a PR to track and propose a way to fix this in core: #13842 |
Co-authored-by: Sarah Rainsberger <[email protected]> Co-authored-by: Matt Kane <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.
CodSpeed Performance ReportMerging #13817 will not alter performanceComparing Summary
|
|
Added a changeset for the changes in |
2137869 to
ba9360a
Compare
|
Merge after #13887 |
Co-authored-by: Matt Kane <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better! No more gambiarra fixes!
…ro#13817) * Clarify and reduce Cloudflare integration logs * Simplify log Co-authored-by: Sarah Rainsberger <[email protected]> * Move enabling sessions log to only show in build * Use double quotes instead * Add option to suppress adapter feature support log * Add fixture to test option * Add changeset * Apply suggestions from code review Co-authored-by: Sarah Rainsberger <[email protected]> Co-authored-by: Matt Kane <[email protected]> * Use `suppress` option instead of hack * Create fast-planets-shout.md * Update .changeset/fast-planets-shout.md Co-authored-by: Matt Kane <[email protected]> --------- Co-authored-by: Sarah Rainsberger <[email protected]> Co-authored-by: Matt Kane <[email protected]>
Changes
This PR makes a few changes for logs in the Cloudflare adapter's dev server based on feedback from withastro/adapters#191 and my experience using it.
Here's the log you would get when starting the dev server with the Cloudflare adapter:
Respectively, here are my thoughts and solutions:
The first log says it's enabling sessions with filesystem storage, but asks you to define a KV binding name, which sounds confusing.
— Changed it so it clarifies it will enable it for production usage, and the follow-up docs PR will add information about the Sessions behavior during dev.
Again, if I am using the filesystem storage, why would that matter?
— Made so this log is only shown during build. That's when you'd get the mentioned error and the information would be most relevant to be shown to you, plus making the dev startup logs one line shorter.
Limited support, ok! How exactly limited, though?
— Clarified that you can still use sharp during build time, but not runtime, meaning only pre-rendered pages will get optimized.
Now its no support at all? Also, this says you can use the
compileimage service, but using it doesn't make the warning go away, and in a kind of funny way, I was totally unaware it actually uses sharp, which I was just told was not compatible at all, so...? 😆— Clarified the
compileoption means using sharp.NOTE: Not really sure if this is a patch or a minor change? 😅
Testing
I am not sure how/if I am supposed to test CLI logs automatically, but for manual testing:
packages/integrations/cloudflarewith this branch's changes.imageServiceoption set.The expected behavior is to get the two warnings about sharp with
@astrojs/cloudflarewhile noimageServiceis set, but when set, it is suppressed. And the tip about the "invalid session storage" to only be shown during the build process.Docs
Sibling docs PR: withastro/docs#11708
/cc @withastro/maintainers-docs for feedback!