fix: remote flag not working for preview command's cache population #888
Conversation
🦋 Changeset detectedLatest commit: bf128dc 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: |
|
Thanks for the fix @james-elicx @dario-piotrowicz could you please take a look as you worked on the remote bindings. |
There was a problem hiding this comment.
Looks good to me 🙂
Although, tangential to the changes here, long term I would say that ideally remote bindings should be used instead of running the worker with --remote (unless there's a benefit/reason to do so?)
I think the plan is to remove |
| environment: args.env, | ||
| wranglerConfigPath: args.wranglerConfigPath, | ||
| cacheChunkSize: args.cacheChunkSize, | ||
| shouldUsePreviewNamespace: args.remote, |
There was a problem hiding this comment.
Should shouldUsePreviewNamespace just be remote / useRemote
| (c) => | ||
| withPopulateCacheOptions(c).option("remote", { | ||
| type: "boolean", | ||
| default: false, |
There was a problem hiding this comment.
-r, --remote Run on the global Cloudflare network with access to production resources [boolean] [default: false]
Add the "r" alias?
| @@ -228,6 +238,7 @@ function populateD1TagCache( | |||
| "d1 execute", | |||
| D1_TAG_BINDING_NAME, | |||
| `--command "CREATE TABLE IF NOT EXISTS revalidations (tag TEXT NOT NULL, revalidatedAt INTEGER NOT NULL, UNIQUE(tag) ON CONFLICT REPLACE);"`, | |||
| ...(populateCacheOptions.shouldUsePreviewNamespace ? ["--preview"] : ["--preview false"]), | |||
There was a problem hiding this comment.
nit:
| ...(populateCacheOptions.shouldUsePreviewNamespace ? ["--preview"] : ["--preview false"]), | |
| ...(populateCacheOptions.shouldUsePreviewNamespace ? ["--preview"] : []), |
Because:
- it's a boolean arg
- to be consistent across the PR
There was a problem hiding this comment.
Same comment applies to l 207
There was a problem hiding this comment.
@james-elicx I added a few minor comments.
The one most important thing is to add the "r" alias for "remote" IMO.
Thanks!
When passing the
--remoteflag to ourpreviewcommand, it would fail to properly populate the cache due to not targeting the remote bindings and missing a--previewflag for Workers KV / D1.fixes #887
Changes:
Tested that cache population was working as expected with the following commands for the
d1-tag-nextexample:opennextjs-cloudflare preview --config wrangler.e2e.jsoncopennextjs-cloudflare preview --config wrangler.e2e.jsonc --remoteopennextjs-cloudflare deploy --config wrangler.e2e.jsonc