-
-
Notifications
You must be signed in to change notification settings - Fork 780
feat: Add Scaleway serverless function preset #3970
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
base: main
Are you sure you want to change the base?
Conversation
|
@Siilwyn is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
π WalkthroughWalkthroughAdds a Scaleway Serverless preset: new preset module and runtime handler, updates preset registry and types, adds Scaleway deployment docs and an integration test, and adds Changes
Estimated code review effortπ― 3 (Moderate) | β±οΈ ~20 minutes π₯ Pre-merge checks | β 2 | β 1β Failed checks (1 warning)
β Passed checks (2 passed)
βοΈ Tip: You can configure your own custom pre-merge checks in the settings. β¨ Finishing touches
π§ͺ Generate unit tests (beta)
No actionable comments were generated in the recent review. π π§Ή Recent nitpick comments
Warning Review ran into problemsπ₯ ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1b2a136 to
f8c6e3a
Compare
|
Moved from draft so CodeRabbit picks it up. |
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.
Actionable comments posted: 1
π€ Fix all issues with AI agents
In `@src/presets/scaleway/runtime/scaleway-serverless.ts`:
- Around line 11-29: In handler, remove non-null assertions on event.headers and
event.queryStringParameters and guard them with safe fallbacks (e.g., const hdrs
= event.headers ?? {}; const qs = event.queryStringParameters ?? {}), iterate
hdrs safely when building headers map, and perform a case-insensitive lookup for
X-Forwarded-Proto (check hdrs["x-forwarded-proto"] and hdrs["X-Forwarded-Proto"]
or normalize keys) when composing the origin for joinURL; also handle body
decoding based on event.isBase64Encoded (decode to string/buffer when true) and
treat both GET and HEAD as having no body when constructing the Request. Ensure
you update references in handler to use these new locals (hdrs, qs, decodedBody)
and remove any remaining ! non-null assertions.
π§Ή Nitpick comments (1)
docs/2.deploy/20.providers/scaleway.md (1)
1-7: Add a minimal config snippet for quicker onboarding.A short
nitro.configexample would make the preset immediately actionable for readers skimming the page.βοΈ Example addition
# Scaleway > Deploy Nitro apps to Scaleway. **Preset:** `scaleway-serverless` +```ts +// nitro.config.ts +export default defineNitroConfig({ + preset: "scaleway-serverless", +}); +``` + :read-more{title="Scaleway Serverless Functions" to="https://www.scaleway.com/en/docs/serverless-functions/"}
β Actions performedReviews resumed. |
β Actions performedFull review triggered. |
| }, | ||
| }, | ||
| { | ||
| name: "scaleway-serverless" as const, |
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.
We might choose a more clearname for this, perhaps We might choose a better name perhaps scaleway-functions?
(since we also have Scaleway serverless containers product)
| headers.get("Host")!, | ||
| event.path | ||
| ), | ||
| event.queryStringParameters ?? {} |
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.
Can you please refactor to use https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams to stringify queryStringParameters? (also don't we have access to original request string?)
| import { useNitroApp } from "nitro/app"; | ||
| import { joinURL, withQuery } from "ufo"; | ||
| import type { serveHandler } from "@scaleway/serverless-functions"; | ||
| import { Buffer } from "node:buffer"; |
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.
Buffer should be available globally
| import { defineNitroPreset } from "../_utils/preset.ts"; | ||
|
|
||
| const scalewayServerless = defineNitroPreset( | ||
| { |
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.
we might enable serve static handler (with inlined contents) or explain it in docs (containers have better support for static assets i guess also)
commit: |
π Linked issue
#2973
β Type of change
π Description
Add Scaleway serverless functions preset.
π Checklist
Other
Open to all feedback, I'm figuring out what to add further to the documentation and preset config so deploys go as easy as possible.