Skip to content

Conversation

@shinchan79
Copy link

@shinchan79 shinchan79 commented Nov 10, 2024

Summary

Add a new tutorial "Content Version Management System using Cloudflare Workers and Durable Objects."

Screenshots (optional)

Documentation checklist

Proposed changes

Added a new tutorial in the src/content/docs/workers/tutorials/build-a-content-version-system directory.

The functions which we will be covering in this tutorial are as follows:

  • Create a new content
  • Update the content
  • Get the content (specific version and list of all versions)
  • Revert to a specific version
  • Publish or unpublish a version
  • Detailed history of modifications
  • Tag a version
  • Delete a version (in case you do not need it anymore)

The functions are implemented on Cloudflare Workers and the states of the contents are managed on Durable Objects.

The tutorial used TypeScript code and Bash commands.
Code snippets and examples adhere to the Cloudflare documentation style guide.

Copy link
Contributor

@hyperlint-ai hyperlint-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 files reviewed, 1 total issue(s) found.

…tem/index.mdx

Co-authored-by: hyperlint-ai[bot] <154288675+hyperlint-ai[bot]@users.noreply.github.com>
Copy link
Contributor

@hyperlint-ai hyperlint-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 files reviewed, 1 total issue(s) found.

…tem/index.mdx

Co-authored-by: hyperlint-ai[bot] <154288675+hyperlint-ai[bot]@users.noreply.github.com>
Copy link
Contributor

@hyperlint-ai hyperlint-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 files reviewed, 1 total issue(s) found.


Note: We resolved prior Hyperlint review comments because:

We updated our inline suggestion AI.

We do this to avoid keeping outdated or irrelevant comments around. We'll leave a new review with current comments below.

@db-cloudflare db-cloudflare added DevRel Tasks that need support from developer relations. developer-spotlight labels Nov 11, 2024
@harshil1712 harshil1712 self-assigned this Nov 14, 2024
Copy link
Contributor

@hyperlint-ai hyperlint-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 files reviewed, 3 total issue(s) found.


Note: We resolved prior Hyperlint review comments because:

We updated our inline suggestion AI.

We do this to avoid keeping outdated or irrelevant comments around. We'll leave a new review with current comments below.

@github-actions github-actions bot added size/xl and removed size/l labels Nov 28, 2024
Copy link
Contributor

@harshil1712 harshil1712 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @shinchan79, this is great tutorial! However, I believe it needs some more work before it can be published. I have left a few comments, and here's my general feedback:

  • Can we have explanation for the code blocks? The comments in the code are excellent, but an explanation outside of that will be helpful
  • I get TypeScript errors when following the flow you describe: adding code to index.ts, handling request in DO, and then adding code functionality in DO. You reference methods and variables that are defined in the later steps. Seeing these errors can confuse the reader. Can we change the order?
  • I am not sure if a front-end is needed for the tutorial. Your deployed link takes me to a Next.js app. However, I don't see that in here. Can you clarify that for me?

Thank you again for the contribution. This is a great tutorial and I am excited to see this get published.

<h2>Current Content:</h2>
<pre>${content}</pre>
</div>
<a href="http://localhost:3000" class="button">Content Version Management</a>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will result in a 404. Is there front-end coding missing with this project?

Copy link
Author

@shinchan79 shinchan79 Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually because of the fact that the tutorial is quite long, I have remove the front end code, and add the link link to front end repo in the README of backend repo: https://github.com/shinchan79/content-version-system/blob/main/README.md#frontend-if-you-need-it-i-deployed-it-to-cloudflare-pages. Maybe I will remove this button if it makes reader confused

Comment on lines 526 to 530
const corsHeaders = {
'Access-Control-Allow-Origin': '*',
'Access-Control-Allow-Methods': 'GET, POST, PUT, DELETE, OPTIONS',
'Access-Control-Allow-Headers': 'Content-Type',
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are handling CORS in the worker, do we really need it here? When the request is made, it first goes through the Worker and only then to the Durable Object

Comment on lines 1123 to 1126
## 6. Additional resources
If you want to implement CI/CD for Worker platform, you can navigate to this blog: [Continuous Deployment for Cloudflare Workers with GitHub Actions](https://blog.cloudflare.com/workers-builds-integrated-ci-cd-built-on-the-workers-platform/)

You can find source code for this project on: [GitHub](https://github.com/shinchan79/content-version-system.git)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While these are good, we should have more resources that can help the reader know about the next steps

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@harshil1712 orry for the late reply. I was sick last week, and only today I'm able to work and update the PR based on your comments.

Please check and let me know if there is anything I should improve. Thank you so much!

@db-cloudflare
Copy link
Contributor

A small suggestion, It might be also worth changing the folder name from build-a-content-version-system to build-a-version-control-system to match the new title.

Copy link
Contributor

@hyperlint-ai hyperlint-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 files reviewed, 3 total issue(s) found.

Copy link
Contributor

@hyperlint-ai hyperlint-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 files reviewed, 3 total issue(s) found.

Update to resolve comments of technical review
@shinchan79
Copy link
Author

@harshil1712 just want to ask if any other things should I improve after my most updated commit?

@harshil1712
Copy link
Contributor

@harshil1712 just want to ask if any other things should I improve after my most updated commit?

Thank you for making the changes. I wasn't sure if it was ready for review. I will review it this week. Thank you!

Copy link
Contributor

@harshil1712 harshil1712 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, I like where we are going with this! I have left a few comments. I was also wondering, do we need the front-end like we have in your finished demo? If so, can we add it in the tutorial? The front-end part can be something they can git clone, maybe? Also, we can use Workers Static Assets to serve the front-end. This will allow us to have it all in a single repo

Comment on lines +71 to +72
[placement]
mode = "smart"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good to have, but not required. If we want to keep it, it would be nice to add one-two line telling the reader what it is and link it to the docs

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@harshil1712 I have read all your comment. Thanks for helping me write a better article. Please give me some days, I will try my best to resolves all the comments next week. Thank you so much!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will also edit a little bit in the front-end repo so that the code is more understandable, and will insert the link in the tutorial for whoever need frontend part.

Comment on lines +99 to +104
"scripts": {
"lint": "npx eslint . --ext .ts --fix",
"format": "prettier --write .",
"test": "vitest run",
"test:watch": "vitest"
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's update to something as follows:

"scripts": {
...
    "lint": "npx eslint . --ext .ts --fix",
    "format": "prettier --write .",
    "test": "vitest run",
    "test:watch": "vitest"
  }

If the reader simply copy pastes this, they might overwrite the existing scripts and run into error when running commands like npm run dev

Comment on lines +304 to +306
type Env = {
CONTENT: DurableObjectNamespace;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When running the command npm run cf-typegen we are already getting this type. We don't need to define it again.

### Initial setup and HTML template

```typescript
import { ContentDO } from './contentDO';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We haven't defined the DO class. This will throw an error. We should move the DO code before this, or add note letting the user know that an error is expected.

```

Key features:
- Sets up CORS headers for cross-origin requests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need CORS? Since we are not calling the worker from another URL, this shouldn't be required.

```

This section:
- Sets up necessary imports and CORS headers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the previous comment on CORS

@kodster28
Copy link
Collaborator

This PR has been sitting for quite a while without any updates (or response to the comments dropped by @harshil1712).

Going to close it out for now. If you want to continue with the work, you'll need to make a new one.

@kodster28 kodster28 closed this May 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

developer-spotlight DevRel Tasks that need support from developer relations. product:workers Related to Workers product size/xl

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants