Skip to content
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

Add readonly mode #243

Closed
wants to merge 2 commits into from
Closed

Conversation

maschwenk
Copy link

In this PR:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Issues reference:

Checklist:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you lint your code with pnpm lint locally prior to submission?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran build with pnpm build of your changes locally?
  • Have you successfully ran tests with pnpm test of your changes locally?
  • Have you commit using Conventional Commits?

@matteovivona
Copy link
Collaborator

@maschwenk Could you follow the conventional commits? Thanks!

@matteovivona matteovivona linked an issue Nov 14, 2023 that may be closed by this pull request
@maschwenk
Copy link
Author

@maschwenk Could you follow the conventional commits? Thanks!

@matteovivona Totally, was just putting this here as a placeholder as I wasn't sure if this was an idea y'all would be on board with at all

@maschwenk
Copy link
Author

@matteovivona would you accept this PR more or less as-is? (after adding testings and conventional commits, obviously)

I opted to not mount the route at all because I want to make sure it 404s instead of 500s or 200s. I believe Turborepo will retry on 500s and 200 ACKing feels kinda wrong.

Worth noting I'm also working with Turborepo to implement this at the Client level, but they have some concerns about how to make that functionality coherent with all the other flags they have

@maschwenk
Copy link
Author

@fox1t @matteovivona The client-side flag actually landed in 1.10.17-canary.8 from

So I think this may not be necessary at all to implement server side. In fact in pretty much every way, it feels like a client side implementation is better.

@fox1t
Copy link
Collaborator

fox1t commented Dec 3, 2023

@fox1t @matteovivona The client-side flag actually landed in 1.10.17-canary.8 from

So I think this may not be necessary at all to implement server side. In fact in pretty much every way, it feels like a client side implementation is better.

OH! Awesome! We can skip to implement it here, even if I can see user cases where can be useful to have a server that can be reachable only in read mode.

@maschwenk
Copy link
Author

@fox1t agreed. The server-side implementation use-case seems a little more fringey.

@maschwenk maschwenk closed this Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support read-only mode to prevent unwanted cache update
3 participants