Repository navigation
Conversation
Problem: On Node 22+, `yarn build` fails for donate-button-v4 and the cloudflare-worker tests fail. Root Cause: preact-cli loads preact.config.js through the archived `esm` package, which throws "Function.prototype.apply was called on undefined" on Node 22+. preact-cli then falls back to its default config, so the `src/...` import alias is missing. preact-cli no longer sees active development and recommends Vite. Separately, Node 21+ defines a getter-only global `navigator`, so `Object.assign(global, makeServiceWorkerEnv())` throws in the worker test setup. Solution: Replace preact-cli with Vite and @preact/preset-vite. The Vite build keeps the output of the old preact.config.js: a single classic-script bundle at dist/donate-button/0.4/index.js, absolute asset URLs from VERCEL_URL or assets.every.org, the demo index.html, and the src/assets copy. Define the worker mocks with Object.defineProperty. Set engines.node, .tool-versions and CI to Node 24. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Name emitted icons with the MD4 digest of their contents, as preact-cli's file-loader did, so the build emits the same 28 icon files at the same URLs as before. Node only exposes MD4 with --openssl-legacy-provider, so the build script sets it. Drop changes the migration does not need: the commented-out CI job, Vite options that equal their defaults, and the build log line. Replace the worker test setup loop with deleting the getter-only navigator before Object.assign. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Node 21+ navigator fix for the cloudflare-worker tests now lives in its own PR, which this PR depends on for the Node 24 test job. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Keep the dev-only entry script in the existing pre-transform HTML hook. The production bundle and HTML retain their existing bytes. Manual check: On Node 24, build v4 and inspect the emitted HTML. Expected: one classic index.js script and no source entry. Actual: matched. Dev HTML transformation emitted one module entry and one Vite client script. Generated with AI under the guidance of John.
Problem The service-worker-mock setup throws while assigning Node 24's read-only navigator global, so no worker tests run. Root cause The mock replaces Node globals during AVA setup and is incompatible with Node 24. Solution Use native Request and Response in the direct Sinon handler tests, with request-shaped CONNECT and TRACE fixtures. Dispatch the built service-worker-format bundle in Miniflare with KV and a mocked outbound fetch. Run the CI test job on Node 24. Manual test Expected: all eight unsupported methods return 404; KV and outbound fetch assertions pass; Miniflare returns the mocked bundle and writes the default bundle URL to KV. Actual: 11 worker tests pass on Node 24, and the root test command passes on Node 24. Package lint on Node 18 reports only the unchanged xo.config.js import/extensions error; changed files pass targeted XO. Docs: https://developers.cloudflare.com/workers/testing/miniflare/
Problem The first Miniflare test migration moved CI tests to Node 24 because Miniflare 4 requires Node 22 or newer. Root cause The selected Miniflare version had a newer Node engine requirement than this repository's CI test job. Solution Pin Miniflare 3.20250718.3, which supports Node 18, and restore the CI test job to Node 18.17.1. Manual test Expected: the built service-worker-format test and all direct handler tests pass on Node 18 and Node 24 with no CI Node-version change. Actual: all 11 worker tests pass on each version; the root test command passes on Node 18. Package lint on Node 18 reports only the unchanged xo.config.js import/extensions error; changed test files pass targeted XO. Docs: https://developers.cloudflare.com/workers/testing/miniflare/
johnemau
marked this pull request as ready for review
September 30, 2026 00:13
johnemau
marked this pull request as draft
September 30, 2026 00:13
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Local preview loads the published bundle, and development mode cannot read script-based options.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
This PR moves the v4 donate-button build from preact-cli to Vite so it can build on newer Node versions while preserving the embed bundle and demo.
Changes:
- Replaces the v4 build configuration and updates its dependencies and lockfile.
- Sets Node 24 for the repository and CI.
| File | Description |
|---|---|
yarn.lock |
Records the Vite dependency changes. |
packages/donate-button-v4/vite.config.mjs |
Configures the bundle, assets, and demo page. |
packages/donate-button-v4/preact.config.js |
Removes the old build configuration. |
packages/donate-button-v4/package.json |
Switches build and development scripts to Vite. |
package.json |
Requires Node 24. |
.tool-versions |
Selects Node 24 locally. |
.github/workflows/ci.yml |
Runs CI on Node 24. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| .readFileSync(HTML_TEMPLATE, 'utf8') | ||
| .replace( | ||
| '</head>', | ||
| `<script type="text/javascript" src="${base}index.js"></script></head>` |
Contributor
There was a problem hiding this comment.
ooh is this true? would be great to preview on demo page before merging
Contributor
Author
There was a problem hiding this comment.
This was an AI slop. It turns out the problem not in the original preact setup, it was a problem created by my first claude prompt and then fixed, my bad!
Problem The added Miniflare integration test expands coverage beyond the requested migration scope. Root cause The earlier test migration added a built-worker dispatch test and its Miniflare dependency to exercise behavior already covered by direct handler tests. Solution Remove the integration test, Miniflare dependency, and build-before-test step. Keep native Request and Response in the existing direct tests and retain the original ten assertions. Manual test Expected: existing handler assertions pass on Node 18 and Node 24 without a new test or CI Node-version change. Actual: ten worker tests pass on both versions, and the root test command passes on Node 18. Package lint still reports only the unchanged xo.config.js import/extensions error; changed files pass targeted XO.
Problem: Node Request rejects CONNECT and TRACE, so those cases need request-shaped fixtures. Root cause: An inline object assertion required a consistent-type-assertions suppression. Solution: Name the fixture before asserting its Request type. Manual test: Expected: All 10 worker tests pass on Node 24, and the edited fixture has no lint suppression. Actual: All 10 tests passed on Node 24; package lint reports only the existing xo.config.js import/extensions error.
Problem: Test requests used the production assets hostname. Root cause: Native Request needs an absolute URL in Node. Solution: Use assets.test for all three test request URLs. Manual test: Expected: All 10 worker tests pass on Node 24 with the reserved .test hostname. Actual: All 10 tests passed; package lint reports only the pre-existing xo.config.js import/extensions error.
Problem: Native Request needs an absolute URL, and the previous test host did not mirror the assets host name. Root cause: The test fixtures used a shorter reserved host. Solution: Use assets.every.org.test in each request fixture. Manual test: Expected: All 10 worker tests pass on Node 24 with the requested reserved test host. Actual: All 10 tests passed; package lint reports only the pre-existing xo.config.js import/extensions error.
Problem: The request-shaped object is a fake request, but its name called it a fixture. Root cause: Native Request rejects CONNECT and TRACE, so those cases use a plain object. Solution: Rename requestFixture to requestFake without changing behavior. Manual test: Expected: All 10 worker tests pass on Node 24 after the rename. Actual: All 10 passed; package lint reports only the pre-existing xo.config.js import/extensions error.
Problem: The worker README says tests mock a service-worker context and use service-worker-mock. Root cause: Test setup changed to Node's native web APIs while the README stayed unchanged. Solution: Describe the direct handler tests and retain the end-to-end testing guidance. Manual test: Expected: README describes the current tests, and all 10 worker tests pass on Node 24. Actual: README matches the test setup; all 10 tests passed on Node 24. Package lint reports only the pre-existing xo.config.js import/extensions error.
2 tasks done
johnemau
marked this pull request as ready for review
October 5, 2026 15:04
Problem: PR #462 and PR #464 both target main, so the Vite migration can be reviewed or merged before its worker test prerequisite. Root cause: Their branches diverged from the same main commit. Solution: Merge the worker test branch into PR #462 and reconcile yarn.lock so the Vite branch contains the worker fix. Manual test: Expected: The combined branch installs with an immutable lockfile, passes all package tests, builds Vite, and passes root lint on Node 24. Actual: yarn install --immutable, yarn test, yarn build, and yarn lint passed on Node 24.16.0. PRs: #464 and #462
johnemau
changed the base branch from
main
to
john-with-claude/miniflare-node24-tests
October 5, 2026 15:15
Problem Developers cannot test explicit, formOnly, or data-every-style options with yarn dev. Root cause Vite runs the entry as a module, where document.currentScript is null. Solution Mark the injected development script and share a development-only fallback lookup. Keep classic-script precedence and remove the fallback from production builds. Manual test Run yarn dev in packages/donate-button-v4. Add ?explicit=1 or ?formOnly=1, or data-every-style, to the marked entry in vite.config.mjs and reload. Expected: manual mode, form-only mode, or styled links respectively. Actual before: options ignored. Actual after: option helpers honor the marked script. Validation Three regression tests fail before the change; all eight pass afterward, including Vite marker injection and environment replacement. Package lint, TypeScript, and production build pass. Production bundle contains no development selector or process.env.NODE_ENV. Reference: #462 (comment) Docs: https://developer.mozilla.org/en-US/docs/Web/API/Document/currentScript Generated with AI under the guidance of John.
johnemau
marked this pull request as draft
October 6, 2026 16:02
Reverts c7e44cc and 1ce09d3. On main, `yarn dev` injected the bundle without query parameters, so explicit, formOnly, and data-every-style could not be set from the dev page either. Removing the fallback returns the v4 source to main's version. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Keep `yarn workspace @everydotorg/donate-button-v4 test` in the root test command and the v4 `echo 'no tests'` script, as on main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Use the Vercel deployment URL as the base when VERCEL_URL is set and "/" otherwise, so development and local builds load from the local server. Serve the demo page at "/" from one middleware and emit it from the same template helper, replacing the request rewrite and the transformIndexHtml hook. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Move the demo page to the package root with a module script for src/index.tsx, so `vite` serves it at "/" with hot reload and no plugin hooks. Move src/assets to public/assets, which Vite serves in development and copies into the build. The build still emits the demo page for embeds.every.org/0.4, swapping the module script for the built index.js. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Use index.html as the build input. Vite replaces its /src/index.tsx script with the built index.js, so the demo-page plugin is no longer needed; index.js is byte-identical to the previous build. Import package.json directly and parse the version with semver, as preact.config.js did, and declare semver as a devDependency. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A bare JSON import makes Vite warn that the config is unsupported by its planned native config loader, and Prettier 2.8 cannot parse the `with { type: "json" }` attribute that silences it.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Prettier 3.0.0 is the first release that parses `import ... with {type: 'json'}`. Pin ~3.0.0 because later 3.x releases reformat 11 more v4 files. The only reformat is the demo page's doctype.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Vite 8 deprecates build.rollupOptions in favor of build.rolldownOptions, and index.html at the project root is already the default build entry. The built files are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Vite 8 resolve.tsconfigPaths reads the baseUrl the code and TypeScript already use for src/ imports, which removes the alias and the package directory lookup. The dev server cannot resolve src/ imports without one of the two. The built index.js is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
johnemau
marked this pull request as ready for review
October 7, 2026 00:16
The worker test fix branch was rebased onto main, bringing in the XO config import fix from #465. Keep this branch's yarn.lock: it already drops the service-worker-mock entries that branch removes, and yarn install --immutable passes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Merged
Restore the cloudflare-worker test setup, tests, README, and service-worker-mock dependencies to main, so the Miniflare migration can stack on this PR instead of under it. Node 21+ defines a getter-only global navigator that Object.assign cannot overwrite; delete it before installing the service-worker-mock globals. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Move donate-button-v4 to Vite 8 and the repo to Node 24 so v4 builds on Node 22+.
Problem
yarn buildfails on Node 22 and 24:Can't resolve 'src/autoPlayMode'.Root Cause
esmloader fails on Node 22+, dropping thesrc/alias.Function.prototype.applyerror.Solution
@preact/preset-vite; see docs: Add notice forcreact-preactpreactjs/preact-cli#1808.VERCEL_URL) as the asset base for production builds and/otherwise.--openssl-legacy-providerand retain the ES2015 target.engines.node,.tool-versions, and CI to Node 24.Visual check
yarn devdemo.Both screenshots were captured at 1280×800 after clicking "Donate". They are byte-identical, so both columns use the same hosted image.
Build output
yarn buildinpackages/donate-button-v4on Node 24.16.0 emits 28 MD4-named icons,index.html, andindex.js(142 kB, 50 kB gzip).button.js(Vercel, gzip)~3K savings by moving from babel to rollup bundlers (preact-cli -> vite)
🤖 Generated with Claude Code