Skip to content

feat(wrangler): DX optimisations in wrangler dev --remote #7425

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

Merged
merged 1 commit into from
Dec 19, 2024

Conversation

CarmenPopoviciu
Copy link
Contributor

@CarmenPopoviciu CarmenPopoviciu commented Dec 3, 2024

Fixes DEVX-1523

Workers + Assets projects have, in certain situations, a relatively degraded wrangler dev --remote developer experience, as opposed to Workers proper projects. This is due to the fact that, for Workers + Assets, we need to make
extra API calls to:

  1. check for asset files changes
  2. upload the changed assets, if any

This commit improves the wrangler dev --remote DX for Workers + Assets, for use cases when the User Worker/assets change while the API calls for previous changes are still in flight. For such use cases, we have put an exit early strategy in place, that drops the event handler execution of the previous changes, in favour of the handler triggered by the
new changes.

For before/after screenshots pls see DEVX-1523

How these changes were tested

NOTE

For posterity, we did discuss internally the unit/e2e test coverage for this PR, and we all agreed that our current dev and dev --remote tests give us enough coverage for these changes. Testing these changes would in fact involve testing a core wrangler functionality, aka aborting in progress changes if there’s a new pending change that would run immediately after, which is not specific to assets. This functionality is already covered by tests, so adding more that are specific for assets doesn't seem to add much value. We therefore decided to merge as is, and rely on existing tests.

  • Workers + Assets (with changes in both assets and User Worker code)

Before
Screenshot 2024-12-19 at 10 51 55

After
Screenshot 2024-12-19 at 10 53 09

  • Workers + Assets only

After
Screenshot 2024-12-19 at 10 54 23

  • Workers proper (no assets)

After
Screenshot 2024-12-19 at 12 58 12


  • Tests
    • TODO (before merge)
    • Tests included
    • Tests not necessary because: we're covered by existing e2e & unit tests here
  • E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required
    • Not required because:
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Documentation not necessary because: watch mode optimizations

@CarmenPopoviciu CarmenPopoviciu requested a review from a team as a code owner December 3, 2024 15:16
Copy link

changeset-bot bot commented Dec 3, 2024

🦋 Changeset detected

Latest commit: e849ab5

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

This PR includes changesets to release 2 packages
Name Type
wrangler Minor
@cloudflare/vitest-pool-workers 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

@CarmenPopoviciu CarmenPopoviciu added the e2e Run wrangler + vite-plugin e2e tests on a PR label Dec 3, 2024
@CarmenPopoviciu CarmenPopoviciu marked this pull request as draft December 3, 2024 15:19
@CarmenPopoviciu CarmenPopoviciu force-pushed the carmen/watch-mode-optimisations branch 2 times, most recently from 34022c7 to e1c8f30 Compare December 3, 2024 15:59
Copy link
Contributor

github-actions bot commented Dec 3, 2024

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

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12409630032/npm-package-wrangler-7425

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

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7425/npm-package-wrangler-7425

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12409630032/npm-package-wrangler-7425 dev path/to/script.js
Additional artifacts:
wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12409630032/npm-package-cloudflare-workers-bindings-extension-7425 -O ./cloudflare-workers-bindings-extension.0.0.0-vab01249e3.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-vab01249e3.vsix
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12409630032/npm-package-create-cloudflare-7425 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12409630032/npm-package-cloudflare-kv-asset-handler-7425
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12409630032/npm-package-miniflare-7425
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12409630032/npm-package-cloudflare-pages-shared-7425
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12409630032/npm-package-cloudflare-unenv-preset-7425
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12409630032/npm-package-cloudflare-vitest-pool-workers-7425
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12409630032/npm-package-cloudflare-workers-editor-shared-7425
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12409630032/npm-package-cloudflare-workers-shared-7425
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12409630032/npm-package-cloudflare-workflows-shared-7425

Note that these links will no longer work once the GitHub Actions artifact expires.


[email protected] includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20241205.0
workerd 1.20241205.0 1.20241205.0
workerd --version 1.20241205.0 2024-12-05

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

@CarmenPopoviciu CarmenPopoviciu added the blocked Blocked on other work label Dec 5, 2024
@CarmenPopoviciu CarmenPopoviciu force-pushed the carmen/watch-mode-optimisations branch 2 times, most recently from 14b81d2 to a1cd8fc Compare December 16, 2024 17:32
@CarmenPopoviciu CarmenPopoviciu removed the blocked Blocked on other work label Dec 16, 2024
@CarmenPopoviciu CarmenPopoviciu marked this pull request as ready for review December 16, 2024 17:38
@CarmenPopoviciu CarmenPopoviciu force-pushed the carmen/watch-mode-optimisations branch 2 times, most recently from d98bc63 to 4fe76bb Compare December 19, 2024 09:00
Workers + Assets projects have, in certain situations, a
relatively degraded `wrangler dev --remote` developer
experience, as opposed to Workers proper projects. This is
due to the fact that, for Workers + Assets, we need to make
extra API calls to:

1. check for asset files changes
2. upload the changed assets, if any

This commit improves the `wrangler dev --remote` DX for
Workers + Assets, for use cases when the User Worker/assets
change while the API calls for previous changes are still in
flight. For such use cases, we have put an exit early strategy
in place, that drops the event handler execution of the
previous changes, in favour of the handler triggered by the
new changes.
@CarmenPopoviciu CarmenPopoviciu force-pushed the carmen/watch-mode-optimisations branch from 4fe76bb to e849ab5 Compare December 19, 2024 09:11
@CarmenPopoviciu CarmenPopoviciu merged commit 8757579 into main Dec 19, 2024
35 of 40 checks passed
@CarmenPopoviciu CarmenPopoviciu deleted the carmen/watch-mode-optimisations branch December 19, 2024 12:07
penalosa pushed a commit that referenced this pull request Jan 10, 2025
Workers + Assets projects have, in certain situations, a
relatively degraded `wrangler dev --remote` developer
experience, as opposed to Workers proper projects. This is
due to the fact that, for Workers + Assets, we need to make
extra API calls to:

1. check for asset files changes
2. upload the changed assets, if any

This commit improves the `wrangler dev --remote` DX for
Workers + Assets, for use cases when the User Worker/assets
change while the API calls for previous changes are still in
flight. For such use cases, we have put an exit early strategy
in place, that drops the event handler execution of the
previous changes, in favour of the handler triggered by the
new changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e Run wrangler + vite-plugin e2e tests on a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants