-
Notifications
You must be signed in to change notification settings - Fork 773
Move mikenomitch/hello-world-containers to templates/containers-template #716
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
6362bb8
to
4b2be6e
Compare
|
Preview link not generated: you must be on a branch, not on a fork. |
@shaunpersad @deloreyj @cmsparks can one of you please take a look? |
@th0m do you need preview images in the package.json? |
defaultPort = 8080; | ||
sleepAfter = "2m"; | ||
envVars = { | ||
MESSAGE: "I was passed in via the container class!", |
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.
super nit - the way this is interpolated the "!" results in a grammar error
Maybe wrap this in internal quotes?
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.
Sounds good, this is how it prints now:
Hi, I'm a container and this is my message: "I was passed in via the container class!", my instance ID is: d037394b-e266-4e55-8b7c-dbd2cb99354d
Yep I submitted this to get early feedback, working on the images now |
Preview link not generated: you must be on a branch, not on a fork. |
688f5a5
to
e5ba5dc
Compare
Preview link not generated: you must be on a branch, not on a fork. |
Preview link not generated: you must be on a branch, not on a fork. |
containers-template/tsconfig.json
Outdated
/* Specify how TypeScript looks up a file from a given module specifier. */ | ||
"moduleResolution": "Bundler", | ||
/* Specify type package names to be included without being referenced in a source file. */ | ||
"types": ["@cloudflare/workers-types/2023-07-01"], |
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 shouldn't use @cloudflare/workers-types
when using worker-configuration.d.ts
This should be ./worker-configuration.d.ts
and @cloudflare/workers-types
should be removed from package.json dependencies
import { Container, loadBalance, getContainer } from "@cloudflare/containers"; | ||
|
||
export class MyContainer extends Container { | ||
defaultPort = 8080; |
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.
might consider adding comments to these options for users who are less familiar with these (and don't read the docs)
MESSAGE: "I was passed in via the container class!", | ||
}; | ||
|
||
override onStart() { |
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.
may be worth adding a comment above these explaining that these are optional
containers-template/src/index.ts
Outdated
|
||
// To route requests to a specific container, | ||
// pass a unique container identifier to .get() | ||
if (pathname.startsWith("/container")) { |
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 consider converting this routing logic to a Hono router. I know this is a hello-world example, but a router is needed to build anything meaningful. New users who use this may not know a router exists and end up going way too far with this style of routing and feeling like Workers is difficult to use.
Defaults make a big difference in user experience
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.
+1 to this. The initial simplicity of path-based routing quickly turns into a stumbling block for new users. We are in the process of adding Hono as a requirement to the Contributor Guidelines.
|
- Replace @cloudflare/workers-types with ./worker-configuration.d.ts in tsconfig.json - Remove @cloudflare/workers-types from package.json dependencies - Add Hono for better routing and developer experience - Add helpful comments for container configuration options - Convert manual pathname parsing to clean Hono route handlers
containers-template/package.json
Outdated
"devDependencies": { | ||
"@cloudflare/workers-types": "^4.20250506.0", | ||
"typescript": "^5.5.2", | ||
"wrangler": "https://pkg.pr.new/wrangler@9098" |
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.
When will we be able to update this to an actual wrangler version?
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.
Later today, wrangler team cutting a release right now
I believe I have addressed your comments, ready for review. |
|
|
Description
Fixes #CC-5506.
Checklist
-template
<!-- dash-content-start -->
and<!-- dash-content-end -->
to designate the Dash readme previewdeploy
commandExample package.json