-
Notifications
You must be signed in to change notification settings - Fork 53
Use deployment id on render #937
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
Tagging OptionsShould a new tag be published when this PR is merged?
|
| const opts = fromRequest(req); | ||
| if (opts.deploymentId && deploymentId) { | ||
| if (opts.deploymentId !== deploymentId) { | ||
| throw badRequest({ |
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.
Is this necessary? What do you think about logging the error but not failing the request?
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.
Perhaps we need to fail the request so a second one can cache the right response. If we dont fail will the "not failure" request be cached?
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.
what if we return max-age 0?
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 already have this issue with assets. If we request an asset on a older deploy, it will give an 404.
It is better to fail fast and let the CDN cache on a successful response, and try to fix this issue with another approach.
| import type { PageParams } from "../mod.ts"; | ||
| import Render, { type PageData } from "./entrypoint.tsx"; | ||
|
|
||
| const deploymentId = Deno.env.get("DENO_DEPLOYMENT_ID"); |
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.
Will it exists in all platforms?
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 already exists in context, shouldn't import it from context?
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 is not on the fresh context (what is used there).
No description provided.