-
Notifications
You must be signed in to change notification settings - Fork 773
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
fix: use cwd for --experiment-enable-local-persistence
#767
Conversation
🦋 Changeset detectedLatest commit: 63883b9 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 |
8d86783
to
fb6547e
Compare
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/2109591903/npm-package-wrangler-767 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/767/npm-package-wrangler-767 Or you can use npx https://prerelease-registry.developers.workers.dev/runs/2109591903/npm-package-wrangler-767 dev path/to/script.js |
This sets up `--experiment-enable-local-persistence` to explicitly use `process.cwd() + wrangler-local-state` as a path to store values. Without it, local mode uses the temp dir that we use to bundle the worker, which gets wiped out on ending wrangler dev. In the future, based on usage, we may want to make the path configurable as well. Fixes #766
fb6547e
to
63883b9
Compare
There is a flaky test/s. I reran the (macos) and it passed. Edit: Found the old run for the workflow |
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.
LGTM!
// as an explicit path, or else it'll use the temp dir | ||
// which disappears when dev ends | ||
const localPersistencePathOrDisableLocalPersistence = enableLocalPersistence | ||
? // Maybe we could make the path configurable as well? |
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.
Configurable via the TOML or a flag?
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.
I like the way miniflare handles things, sure could be under another key to not expose the "miniflare" name, but the way it's handled there (default or specific path per KV, DO and cache) is pretty neat.
A CLI flag is also important IMO due to integration test ci/cd scenarios, where loading a test data set is important, and dynamic cache folder paths quickly become cumbersome with wrangler.toml
This sets up
--experiment-enable-local-persistence
to explicitly useprocess.cwd() + wrangler-local-state
as a path to store values. Without it, local mode uses the temp dir that we use to bundle the worker, which gets wiped out on ending wrangler dev. In the future, based on usage, we may want to make the path configurable as well.Fixes #766