Replaced `--experimental-enable-local-persistence` with `--persist` and `-persist-to`

[geelen]

trafficstars

If you try to use --experimental-enable-local-persistence now, you'll get this message:

▲ [WARNING] --experimental-enable-local-persistence is deprecated.

  Use --persist in future (which will write to .wrangler/state) or
  --persist-to=./wrangler-local-state if you have existing data 
  you'd like to keep using.

• --persist-to takes a path relative to process.cwd() (and implies --persist, ofc).
• Otherwise, --persist will use the location of the nearest wrangler.toml and derive .wrangler/state from that.

Went with .wrangler rather than hiding in node_modules as imo you really don't want to lose your local data when you hit the rm -rf node_modules button. Imo, same argument applies to the configCache stuff—we should use .wrangler/config instead of node_modules/.cache/wrangler too, just to keep all our stuff in one place. But that can be a shed for a different bike.

Thoughts on naming? The above warning messaging? Should we store some of this stuff in wrangler.toml somewhere?

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: 522e5d8df9e0002c366d8cbb1c7c53b355dc906a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
wrangler	Patch

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

[github-actions[bot]]

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/3044388510/npm-package-wrangler-1644

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/1644/npm-package-wrangler-1644

Or you can use npx with this latest build directly:

npx https://prerelease-registry.developers.workers.dev/runs/3044388510/npm-package-wrangler-1644 dev path/to/script.js

[codecov[bot]]

Codecov Report

Merging #1644 (522e5d8) into main (874f0a3) will increase coverage by 0.00%. The diff coverage is 44.44%.

@@           Coverage Diff           @@
##             main    #1644   +/-   ##
=======================================
  Coverage   76.21%   76.21%           
=======================================
  Files         104      104           
  Lines        7295     7296    +1     
  Branches     1912     1911    -1     
=======================================
+ Hits         5560     5561    +1     
  Misses       1735     1735           

Impacted Files	Coverage Δ
packages/wrangler/src/api/dev.ts	57.89% <ø> (ø)
packages/wrangler/src/dev/local.tsx	30.67% <ø> (+0.55%)	:arrow_up:
packages/wrangler/src/dev/start-server.ts	71.55% <ø> (+0.12%)	:arrow_up:
packages/wrangler/src/pages/dev.tsx	22.95% <0.00%> (-0.26%)	:arrow_down:
packages/wrangler/src/dev.tsx	84.43% <57.14%> (-1.08%)	:arrow_down:
...ackages/wrangler/src/__tests__/helpers/mock-bin.ts	100.00% <0.00%> (+5.26%)	:arrow_up:

[mrbbot]

Like the flag name. I think you should drop in future from the warning message since we're recommending this now.

~~What do you think about merging --persist-to into --persist, kinda like Miniflare does at the moment? Either --persist on its own to persist to the default path or --persist path/to/persist for --persist-to. Does yargs support this type of option? Potential issue with this though is what happens in the case of wrangler dev --persist my-worker-entrypoint.js?~~ Ah, just seen the internal chat. 😅

[mrbbot]

Could we hold off on merging this right now? I'm planning some changes to the persistence format in the next version of Miniflare, and keeping this flag experimental will avoid making technically semver-major breaking changes in Wrangler.

[geelen]

What kinds of changes are you considering, and how far away are they do you think? I needed --persist-to for the D1 branch, which we will want to merge sooner or later, but doesn't need to happen yet...

[mrbbot]

https://github.com/cloudflare/miniflare/pull/328 (and more broadly using IDs as opposed to binding names for KV/R2 storage).

Also potentially switching everything over to SQLite too to fix issues like this https://github.com/cloudflare/miniflare/issues/167. I'm kinda torn on this though: I really like how you can just see and edit all your keys as files with namespaces separated by directories, but not being able to store foo and foo:bar isn't great. Could go for something flat (i.e. use _ instead of / for namespace separators), but SQLite has other advantages (e.g. much more flexible querying, not loading everything into memory for list()s, etc). What do you think?

As for timeline, I'll DM you.

[geelen]

Talking offline with @mrbbot, the concern is that the internal persistence format might change in a future miniflare release, and so taking it out of "experimental" before then might be premature. Which is a fair consideration, but the current format is extremely useful and both changes (move to SQLite for KV and changing filenames) should be fully automatable.

Personally, I think that leaving things as experimental doesn't help us as much as we'd want—people are still going to depend on it as it's genuinely working well right now. The thing we need is a migration strategy—a way for Miniflare vNext to detect an old-style persistence and uplift it into the new format, whatever that is.

Perhaps we make the state directory record a .version file (or equivalent, elsewhere):

.wrangler/
  state/
    .version

The contents of which could just be 1 or {"version":1}, whatever. But if that file isn't present:

if (versionFileMissing) {
  const { migrator } = await npxImport('@miniflare/persistence-migrator')
  await migrator.zeroToOne(storagePath)
}

That way we don't have to ship the migrator code with every future version of Miniflare, we can just pull it in and run it if we need it. And it gives us a way to migrate storage formats again in the future...