-
Notifications
You must be signed in to change notification settings - Fork 73
refactor: retrieve cache handler kv instance inside constructor #72
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
🦋 Changeset detectedLatest commit: 0dbf79c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
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.
Thanks for the PR.
It looks great with a few minor comments.
A bit off-topic to this PR but more details on our plan since you are a major contributor to this repo:
The plan is that we move/re-use code from opennextjs-aws.
By sharing code and architecture both repos will benefit for the fixes/features.
The first notable change will be that we will run the routing + middleware layer that is not currently supported in this repo.
I hope to have some code ready later this week.
Thanks for all you contribs!
packages/cloudflare/src/cli/build/utils/copy-prerendered-routes.ts
Outdated
Show resolved
Hide resolved
packages/cloudflare/src/cli/templates/cache-handler/open-next-cache-handler.ts
Show resolved
Hide resolved
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.
Thanks!
__NEXT_PRIVATE_STANDALONE_CONFIG?: string; | ||
SKIP_NEXT_APP_BUILD?: string; | ||
NEXT_PRIVATE_DEBUG_CACHE?: string; | ||
__OPENNEXT_KV_BINDING_NAME: string; |
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.
@james-elicx Is this documented already?
If not, would you mind sending a PR?
Also maybe add a default value.
Thanks!
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.
The intention was for this to just be used internally to inline a value into the cache handler, but yeah, it would make sense to let users use it as well
Context
At the moment, the KV instance that is used by the cache handler is assigned to the class as a static property as part of the code patch that happens. We should move this to happen during the creation of a new instance of the class.
Normally, all logic related to setting up the cache handler would happen within the cache handler itself, whereas it currently happens externally. This also means that in the long-term, it makes it easier to extend the cache handler to support additional mechanisms/customisability should we desire (e.g. R2).
Changes