Skip to content
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

[WIP] Implement dev registry as background daemon #4241

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mrbbot
Copy link
Contributor

@mrbbot mrbbot commented Oct 19, 2023

May fix some of the comments in #3121

What this PR solves / how to test:

Alternative to handoff, ensures if original process that started the dev registry is terminated, registry still works for remaining workers and if original worker restarted

Associated docs issue(s)/PR(s):

  • [insert associated docs issue(s)/PR(s)]

Author has included the following, where applicable:

Reviewer is to perform the following, as applicable:

  • Checked for inclusion of relevant tests
  • Checked for inclusion of a relevant changeset
  • Checked for creation of associated docs updates
  • Manually pulled down the changes and spot-tested

Note for PR author:

We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label highlight pr review so future reviewers can take inspiration and learn from it.

@changeset-bot
Copy link

changeset-bot bot commented Oct 19, 2023

⚠️ No Changeset found

Latest commit: 06faf78

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Oct 19, 2023

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/6657701912/npm-package-wrangler-4241

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6657701912/npm-package-wrangler-4241 dev path/to/script.js
Additional artifacts:
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6657701912/npm-package-cloudflare-pages-shared-4241

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


wrangler@3.14.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare 3.20231025.0 3.20231025.0
workerd 1.20231025.0 1.20231025.0
workerd --version 1.20231025.0 2023-10-25

|

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

@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Merging #4241 (06faf78) into main (3d55f96) will increase coverage by 0.10%.
Report is 1 commits behind head on main.
The diff coverage is 84.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4241      +/-   ##
==========================================
+ Coverage   75.39%   75.50%   +0.10%     
==========================================
  Files         223      224       +1     
  Lines       12330    12343      +13     
  Branches     3186     3189       +3     
==========================================
+ Hits         9296     9319      +23     
+ Misses       3034     3024      -10     
Files Coverage Δ
packages/wrangler/src/deploy/deploy.ts 90.00% <ø> (ø)
packages/wrangler/src/deployment-bundle/bundle.ts 88.88% <ø> (-1.02%) ⬇️
packages/wrangler/src/dev-registry/constants.ts 100.00% <100.00%> (ø)
packages/wrangler/src/dev/start-server.ts 79.09% <92.30%> (ø)
packages/wrangler/src/dev/local.tsx 22.22% <55.55%> (-0.70%) ⬇️
packages/wrangler/src/dev/miniflare.ts 61.70% <68.18%> (+0.85%) ⬆️
packages/wrangler/src/dev-registry/index.ts 89.41% <89.41%> (ø)

... and 1 file with indirect coverage changes

@mrbbot
Copy link
Contributor Author

mrbbot commented Nov 3, 2023

Holding off tidying up and merging this until #4175 merged. 👍

petebacondarwin added a commit that referenced this pull request Nov 21, 2023
Reenable when #4241 lands and improves reliability of this test.
petebacondarwin added a commit that referenced this pull request Nov 21, 2023
Reenable when #4241 lands and improves reliability of this test.
petebacondarwin added a commit that referenced this pull request Nov 22, 2023
Reenable when #4241 lands and improves reliability of this test.
petebacondarwin added a commit that referenced this pull request Nov 23, 2023
Reenable when #4241 lands and improves reliability of this test.
petebacondarwin added a commit that referenced this pull request Nov 24, 2023
Reenable when #4241 lands and improves reliability of this test.
petebacondarwin added a commit that referenced this pull request Nov 24, 2023
Reenable when #4241 lands and improves reliability of this test.
petebacondarwin added a commit that referenced this pull request Nov 24, 2023
* fix: ensure C3 shell scripts work on Windows

Our use of `shell-quote` was causing problems on Windows where it was
escaping character (such as `@`) by placing a backslash in front.
This made Windows think that such path arguments, were at the root.

For example, `npm install -D @cloudflare/workers-types` was being converted to
`npm install -D \@cloudflare/workers-types`, which resulted in errors like:

```
npm ERR! enoent ENOENT: no such file or directory, open 'D:\@cloudflare\workers-types\package.json'
```

Now we just rely directly on the Node.js `spawn` API to avoid any shell quoting
concerns. This has resulted in a slightly less streamlined experience for people
writing C3 plugins, but has the benefit that the developer doesn't have to worry
about quoting spawn arguments.

Closes #4282

* C3 e2e - test on CI with Windows + pnpm

* C3 e2e - clean up test projects according to their type

Previously we only deleted Pages projects - in fact we were trying to delete
the Hono (non-Pages) Worker as a Pages project.

Now we delete the project based on its type.

* C3 - framework template fixes for Windows

- do not use bash-style environment variable setting in Docusaurus scripts
- do not run TTY interactive e2e tests on Windows

* C3 e2e - make cleaning up e2e test files more resilient

* C3 - update Nuxt template to avoid requiring Windows incompatible shell environment variable

* C3 e2e - remove noise from expected test errors

* C3 e2e - Only cleanup workers and projects that are over 1 hour old

These e2e test workers and projects should be cleaned up as part of the normal test completion.
But if the test crashes they may be left orphaned.
This change ensures that we do not clean up projects to early while they are still being used.

* C3 e2e - only clean up test projects once per day at 3am

* C3 e2e - fix commit message tests on Windows

* C3 e2e - recreate test folders for each e2e retry

Previously we were reusing folders after clearing them when retrying a
failed test. But this could lead to problems, especially on Windows where
clearing out the folder did not always work.

* C3 e2e - bump test timeout longer

* C3 e2e - do not run C3 e2e tests on unsupported OSes

* C3 e2e - store e2e logs separately by OS

* C3 e2e - Retry some of the checks a few times to reduce flakiness

* C3 e2e - Record vitest results as a json artifact for downloading

* C3 e2e - disable global caches for package managers

This commit uses environment variables to tell package managers to put
cached files in a local directory rather than a global shared one, which
can cause problems with race conditions when running multiple installs
at the same time.

* C3 e2e - don't cancel other matrix jobs if one fails

It is a false optimization to cancel jobs that are likely to pass when
another job flakes out.

* ci: retry the external-durable-objects-app fixture if it flakes

* C3 e2e - drop docusaurus and react tests

These jobs tend to flake out and are not providing much of a useful signal.

* ci: skip fixture tests that exercise the "dev registry"

Reenable when #4241 lands and improves reliability of this test.

* C3 - ensure shell commands in logging are reasonably quoted

C3 often outputs log messages to the user of commands that are being
executed. Users tend to cut and paste these into their terminal to run
themselves. This makes sure that these are likely to just work went
pasted into their shell.

* fixup! C3 - ensure shell commands in logging are reasonably quoted

* fixup! C3 - ensure shell commands in logging are reasonably quoted
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Untriaged
Development

Successfully merging this pull request may close these issues.

None yet

1 participant