Skip to content

Conversation

@trieloff
Copy link
Contributor

Summary

This PR introduces EdgeESBuildBundler as an alternative to the webpack-based EdgeBundler, building upon and expanding the work in PR #89.

  • New EdgeESBuildBundler: Uses ESBuild (already available via @fastly/js-compute) for faster bundling with platform: 'browser' for Service Worker compatibility
  • CLI integration: Support for --bundler esbuild flag to select the new bundler
  • Fastly module handling: ESBuild plugin to properly externalize fastly:* modules
  • Local runtime validation: Integration tests that verify bundles work with both Cloudflare wrangler and Fastly viceroy local runtimes

Key Changes

  • Add src/EdgeESBuildBundler.js extending BaseBundler directly
  • Add test fixture test/fixtures/esbuild-action/ for testing
  • Add integration tests in test/build.esbuild.test.js
  • Add fs-extra and esbuild as direct dependencies

Test plan

  • Unit tests pass (55 passing)
  • ESLint passes (0 errors)
  • Local Cloudflare wrangler validation (detects "cloudflare" platform)
  • Local Fastly viceroy validation (detects "fastly" platform)
  • CI pipeline tests

🤖 Generated with Claude Code

claude and others added 30 commits November 24, 2025 09:22
Fixes #80

BREAKING CHANGE: Replaces Dictionary API with SecretStore and ConfigStore. Existing deployments using Dictionary will need to be redeployed to use the new store types. The deployment process now creates SecretStore for action parameters and ConfigStore for package parameters instead of a Dictionary.

## Changes

### Runtime Adapter (src/template/fastly-adapter.js)
- Replace deprecated `Dictionary` API with modern `SecretStore` and `ConfigStore`
- Update `context.env` Proxy to try SecretStore first (async), then ConfigStore (sync)
- Maintain gateway fallback for dynamic package params
- Update global type declarations

### Deployment Logic (src/ComputeAtEdgeDeployer.js)
- Add helper methods for idempotent store creation:
  - `getOrCreateSecretStore()` - Create or retrieve secret store
  - `getOrCreateConfigStore()` - Create or retrieve config store
  - `linkResource()` - Link stores to service versions
  - `putSecret()` - Add/update secrets
  - `putConfigItem()` - Add/update config items

- **deploy() method**:
  - Create/link SecretStore for action params and special params
  - Create/link ConfigStore for package params
  - Populate both stores during initial deployment

- **updatePackage() method** (CRITICAL BUG FIX):
  - Now handles BOTH action params AND package params
  - Previously only handled action params - package params were ignored!
  - Write action params to SecretStore
  - Write package params to ConfigStore

### Test Updates (test/fastly-adapter.test.js)
- Add mock classes for SecretStore and ConfigStore
- Update tests to use mocked stores
- All unit tests pass ✓

## Parameter Mapping
- **Action parameters** (\`-p FOO=bar\`) → SecretStore (sensitive)
- **Package parameters** (\`--package.params HEY=ho\`) → ConfigStore (non-sensitive)
- **Special parameters** (\`_token\`, \`_package\`) → SecretStore (gateway fallback)

## Benefits
1. Uses modern, non-deprecated Fastly APIs
2. Fixes critical bug where package parameters were not being set
3. Properly separates secrets from config
4. Maintains backward compatibility with gateway fallback
5. Idempotent store creation prevents errors on re-deployment

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- Fix string quotes (use single quotes instead of double)
- Replace await-in-loop with Promise.all for parallel execution
- Fix max-len violations by breaking long lines
- Add eslint-disable for max-classes-per-file in test mocks

All unit tests still passing (13/13).

Signed-off-by: Lars Trieloff <lars@trieloff.net>
…ource

The @adobe/fastly-native-promises library doesn't expose a request() method.
Using this.fetch() with full URL and headers instead.

Fixes integration test error: 'this._fastly.request is not a function'

Signed-off-by: Lars Trieloff <lars@trieloff.net>
Implement proper parameter precedence with two SecretStores:
- Action SecretStore (action-specific params, highest priority)
- Package SecretStore (package-wide params, lower priority)

Changes:
- Create action and package SecretStores with distinct names
- Link both stores to service at deployment
- Update deploy() to populate both stores independently
- Update updatePackage() to update both stores
- Update runtime Proxy to check action_secrets then package_secrets
- Remove ConfigStore usage (all params now in SecretStores)

This ensures action parameters always override package parameters
while maintaining backward compatibility with gateway fallback.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

Signed-off-by: Lars Trieloff <lars@trieloff.net>
Remove the deprecated gateway fallback for parameter resolution.
All parameters are now exclusively stored in and retrieved from
SecretStores (action_secrets and package_secrets).

BREAKING CHANGE: The gateway fallback mechanism has been completely
removed. Applications must use SecretStores for all parameters.
The following are no longer supported:
- Gateway backend configuration
- _token and _package special parameters
- Runtime fallback to gateway for missing parameters
- packageParams caching

This change requires Fastly Compute@Edge with resource bindings
support and is incompatible with older gateway-based deployments.

Migration: Redeploy all actions to ensure parameters are stored
in the new SecretStore architecture.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

Signed-off-by: Lars Trieloff <lars@trieloff.net>
Remove parameter storage and retrieval functionality from FastlyGateway
as it is no longer needed with SecretStore-based parameter management.

Removed methods:
- updatePackage() - stored params in 'packageparams' dictionary
- listPackageParamsVCL() - generated VCL to serve params as JSON

Removed infrastructure:
- 'tokens' dictionary (stored auth tokens)
- 'packageparams' dictionary (stored package parameters)
- 'packageparams.auth' VCL snippet (auth validation)
- Package params error handler VCL snippet

BREAKING CHANGE: FastlyGateway.updatePackage() and
FastlyGateway.listPackageParamsVCL() methods have been removed.
The gateway no longer stores or serves package parameters via
dictionaries. All parameter management must use SecretStores.

FastlyGateway now focuses solely on:
- Request routing between edge backends
- Version alias management
- URL rewriting
- Logging aggregation

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- adds support for Secret Store, Config Store, and Resource Linking APIs
- enables replacement of deprecated Dictionary API with modern alternatives
- provides new functions for managing secrets and configuration in Compute@Edge

Signed-off-by: Lars Trieloff <lars@trieloff.net>
…APIs

- replace custom Secret Store API implementation with writeSecretStore()
- replace custom Config Store API implementation with writeConfigStore()
- replace custom Resource Linking API implementation with writeResource()
- replace custom putSecret() with native putSecret()
- replace custom putConfigItem() with native putConfigItem()
- simplify code by removing manual HTTP requests and using library functions

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- remove getOrCreateSecretStore, getOrCreateConfigStore, linkResource, putSecret, putConfigItem wrapper functions
- call fastly-native-promises methods directly for cleaner code
- maintain same functionality with less indirection
- all tests continue to pass

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- integration tests expect updatePackage method to exist on gateway
- add no-op implementation since FastlyGateway no longer manages package parameters
- package parameters are now handled by individual deployers (ComputeAtEdgeDeployer)

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- catch and ignore 'Duplicate link' errors when creating resource links
- allows redeployment to same service version without failing
- log debug message when resource link already exists
- fixes integration test failures due to existing resource links

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- regenerated package-lock.json to resolve conflicts with main branch
- ensures consistent dependency resolution
- maintains fastly-native-promises 3.1.0 with new Secret Store, Config Store, and Resource Linking APIs

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- resolve package.json conflicts by keeping fastly-native-promises 3.1.0 and updating js-compute to 3.35.2
- regenerate package-lock.json for consistency
- merge latest main branch changes including context.log implementation and CacheOverride API

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- add eslint-disable-next-line comments for legitimate console.log usage in adapters
- ignore test/tmp directory in eslint config to prevent linting generated files
- console statements in adapters are used for error handling and environment detection

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- add type check for non-string properties to prevent Symbol access issues
- add catch block for Promise rejections in secret store access
- improve error logging with property name for better debugging
- prevents function crashes when accessing env properties

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- merge logging example functionality into edge-action fixture to reduce deployment time
- add comprehensive logging test route with operation=verbose parameter
- test both CacheOverride API and logging functionality in single deployment
- verify Secret Store/Config Store implementation works correctly
- function successfully accesses both action params (FOO=bar) and package params (HEY=ho)
- logging functionality returns proper JSON response with status, logging enabled, and timestamp

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- replace httpbin.org with https://www.aem.live/ to eliminate flaky external dependency
- update CacheOverride test routes to use reliable endpoint while maintaining functionality
- use content-length header instead of UUID for response validation
- ensures integration tests are stable and not dependent on external service availability
- all CacheOverride functionality (TTL, pass mode, custom cache key) still properly tested
- test now passes consistently: ✔ Deploy a pure action to Compute@Edge and test CacheOverride API

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- extract worker name from request URL hostname (e.g., 'simple-package--simple-project')
- use real Cloudflare Workers API instead of made-up environment variables
- provide consistent context.func.name behavior across both platforms
- improve debugging by showing meaningful function identifiers in responses
- fallback to 'cloudflare-worker' if URL parsing fails
- update integration tests to expect correct function names
- both platforms now show function identifiers: Fastly (service ID), Cloudflare (worker name)
- fix linting issues: line length, unused variables, console statements

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- update test assertions to only accept successful responses (200, 201)
- reject 503 Service Unavailable and other error responses
- switch to jsonplaceholder.typicode.com for more reliable backend testing
- tests now properly fail when backend requests return errors
- reveals real issue: Fastly returning 503 for external requests (needs investigation)
- Cloudflare tests pass with 200 responses, Fastly tests correctly fail with 503

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- combine unit and integration test coverage in single CI run
- fix CacheOverride import issue in edge-action fixture with mock fallback
- clean up temporary test files that had old imports
- add test-all npm script for running all tests with combined coverage
- move codecov upload after all tests complete to capture full coverage
- increase coverage from 18% to ~71% by including integration test coverage
- resolve issue where codecov only received unit test coverage

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- fail fast with clear error messages when HLX_FASTLY_AUTH is missing
- fail fast with clear error messages when CLOUDFLARE_AUTH is missing
- prevent silent failures or incomplete test coverage in CI
- ensure all integration tests require proper GitHub repository secrets
- provide actionable error messages for CI setup issues
- maintain high test coverage by ensuring all tests actually run

Signed-off-by: Lars Trieloff <lars@trieloff.net>
…dling

SECURITY FIX:
- remove .env file with hardcoded API tokens from repository
- add .env.example with documentation for local development

CI FIX:
- modify dotenv loading to respect existing environment variables
- ensure GitHub Actions secrets take precedence over .env files
- fix integration tests to use CI-provided credentials properly
- prevent .env from overriding GitHub Actions environment variables

This ensures:
- CI uses GitHub repository secrets (HLX_FASTLY_AUTH, CLOUDFLARE_AUTH)
- Local development can use .env files when CI vars aren't set
- No hardcoded credentials in repository
- Proper coverage reporting with actual CI credentials

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- edge integration test is already included in integration-ci
- npm run integration-ci includes all tests with 'Integration' in the name
- edge-integration.test.js is automatically included as 'Edge Integration Test'
- removes script duplication and potential confusion
- maintains full coverage of Secret Store/Config Store functionality in CI

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- remove test/fixtures/logging-example/ directory entirely
- remove skipped logging-example test from cloudflare.integration.js
- update TEST_COVERAGE.md to reflect current test structure
- focus on edge-action fixture which tests Secret Store/Config Store
- remove outdated references to maintain clean codebase

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- remove TEST_COVERAGE.md file (redundant with actual test files)
- remove test-all npm script (unnecessary complexity)
- update CI to run npm test and npm run integration-ci separately
- maintain separate coverage collection for unit and integration tests
- upload combined coverage to codecov after both test runs complete
- cleaner separation of concerns between unit and integration testing

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- Platform detection now happens in edge-index.js based on request.cf
  for Cloudflare and fastly:env module availability for Fastly
- Removed detection functions from cloudflare-adapter.js and fastly-adapter.js
- Adapters now just export handleRequest without detection logic
- Updated cloudflare-adapter tests to remove detection-related tests

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- Created fastly-runtime.js with getFastlyEnv(), getSecretStore(), getLogger()
- These functions dynamically import fastly:* modules
- fastly-adapter.js now imports from fastly-runtime.js instead of
  directly importing fastly:* modules
- This allows proper unit testing with esmock

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- Added function-based external to match all fastly:* modules
- This allows any Fastly runtime module to be externalized without
  needing to explicitly list each one
- Removes need for webpackIgnore comments in source files

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- Added esmock dependency for ES module mocking
- Rewrote fastly-adapter.test.js to use esmock to mock fastly-runtime.js
- Tests now properly exercise handleRequest with mocked Fastly modules
- Added tests for env proxy, secret store access, context structure
- Excluded fastly-runtime.js from coverage (always mocked in tests)

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- Import CacheOverride from @adobe/fetch instead of using mock class
- Replace jsonplaceholder.typicode.com URLs with www.aem.live
- Removes dependency on third-party test services

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- Changed test describe from "Platform" to "Platform Integration"
- Ensures tests are properly included/excluded by -g Integration flag

Signed-off-by: Lars Trieloff <lars@trieloff.net>
Use multiple --target arguments to deploy to both platforms in a single
CLI invocation rather than running two separate deployment functions.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

Signed-off-by: Lars Trieloff <lars@trieloff.net>
The Fastly WASM runtime lacks document.currentScript, causing webpack's
automatic publicPath detection to fail with "Automatic publicPath is not
supported in this browser". Setting publicPath to empty string fixes this.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- Add .toLowerCase() to Cloudflare worker and KV namespace names
- Add duplicate handling for Fastly secret store creation race condition
- Remove explicit package.name override from edge integration test

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

Signed-off-by: Lars Trieloff <lars@trieloff.net>
Fastly's WASM runtime doesn't support importScripts which webpack uses
for code splitting. Changes:
- Convert dynamic adapter imports to static imports in edge-index.js
- Add webpackIgnore comments to fastly:* module dynamic imports
- Add splitChunks: false to webpack optimization

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

Signed-off-by: Lars Trieloff <lars@trieloff.net>
Introduces EdgeESBuildBundler as an alternative to the webpack-based EdgeBundler.
This leverages ESBuild (already a dependency via @fastly/js-compute) for faster
bundling with platform: 'browser' for Service Worker compatibility.

Key changes:
- Add src/EdgeESBuildBundler.js extending BaseBundler directly
- Support for --bundler esbuild CLI flag
- Handle fastly:* external modules via esbuild plugin
- Add test fixture and integration tests for Cloudflare wrangler and Fastly viceroy
- Add fs-extra and esbuild as direct dependencies

Tested locally with both Cloudflare wrangler and Fastly viceroy runtimes.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

Signed-off-by: Lars Trieloff <lars@trieloff.net>
BREAKING CHANGE: The webpack-based EdgeBundler has been removed. All edge
bundling now uses ESBuild via EdgeESBuildBundler. Users must update their
CLI invocations from `--bundler webpack` to `--bundler esbuild`.

- Remove src/EdgeBundler.js
- Remove @adobe/helix-deploy-plugin-webpack peer dependency
- Update all tests to use esbuild bundler
- ESBuild provides faster bundling with smaller output

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

Signed-off-by: Lars Trieloff <lars@trieloff.net>
@github-actions
Copy link

github-actions bot commented Dec 1, 2025

This PR will trigger a major release when merged.

Copy link
Contributor Author

@trieloff trieloff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs work.

package.json Outdated
"@fastly/js-compute": "3.35.2",
"chalk-template": "1.1.2",
"constants-browserify": "1.0.0",
"esbuild": "^0.25.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"esbuild": "^0.25.0",
"esbuild": "0.25.0",

prefer exact versions

Comment on lines 237 to 240
validateBundle() {
// TODO: validate edge bundle
// Could potentially use wrangler/viceroy for validation
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea. implement that, if they are installed

@@ -0,0 +1,194 @@
/*
* Copyright 2021 Adobe. All rights reserved.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Copyright 2021 Adobe. All rights reserved.
* Copyright 2025 Adobe. All rights reserved.

wrong year

claude added 11 commits December 1, 2025 10:44
The mocha exit: true flag ensures mocha exits after tests complete,
even if there are unclosed connections or handles.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- Simplify fetch polyfill to use globalThis.fetch directly instead of
  capturing it at module init time. This fixes issues with ESBuild
  module initialization order where globalThis.fetch wasn't available
  when the polyfill was loaded.
- Fix cloudflare-adapter.js to handle missing PACKAGE binding gracefully
  by checking if target.PACKAGE exists before calling .get()
- Update cache-override tests to match simplified API (initNative/native
  instead of getNative)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

Signed-off-by: Lars Trieloff <lars@trieloff.net>
Add /env-detect endpoint to diagnose platform adapter issues by reporting:
- Runtime and func from context (set by adapter)
- Request indicators (req.cf presence, colo)
- Global environment checks (caches.default, globalThis.fetch)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

Signed-off-by: Lars Trieloff <lars@trieloff.net>
Add functions to create and manage Fastly dynamic backends:
- getBackendClass() - Lazily imports fastly:backend module
- enableDynamicBackends() - Enables dynamic backends via fastly:experimental
- getBackend(hostname) - Gets or creates backend for a hostname:
  - First checks for named backend via Backend.exists()
  - Falls back to creating dynamic backend with SSL/SNI configured
  - Caches created backends to avoid recreation

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

Signed-off-by: Lars Trieloff <lars@trieloff.net>
Enhance fetch polyfill with Fastly-specific improvements:
- Separate environment detection (fastly:env) from CacheOverride loading
- Add getHostname() helper to extract hostname from URL/Request
- Automatically resolve backends when none provided using getBackend()
- Properly merge backend and cacheOverride options in fetch calls

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

Signed-off-by: Lars Trieloff <lars@trieloff.net>
Add console.log statements throughout to trace:
- Module loading lifecycle
- Platform detection logic (request.cf, fastly:env import)
- Handler resolution and fetch event handling

This aids in diagnosing adapter selection issues.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

Signed-off-by: Lars Trieloff <lars@trieloff.net>
The fetch polyfill was doing its own dynamic import of fastly:env
for platform detection, which failed in Fastly's WASM context with
"Dynamic module import is disabled or not supported in this context".

Now the fetch polyfill imports getFastlyEnv from fastly-runtime.js,
which already handles the Fastly module imports correctly and caches
the results. This eliminates the duplicate detection and ensures
consistent behavior across both Cloudflare and Fastly platforms.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

Signed-off-by: Lars Trieloff <lars@trieloff.net>
- Remove fastly-runtime.js from test coverage exclusion
- Use exact version for esbuild dependency (no caret)
- Fix copyright years to 2025 across affected files
- Add validateBundle() method using wrangler/fastly CLI
- Use fastly-runtime.js for logger import to avoid webpackIgnore

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

Signed-off-by: Lars Trieloff <lars@trieloff.net>
🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

Signed-off-by: Lars Trieloff <lars@trieloff.net>
…quest

- Replace dry-run validation with actual server startup
- Start wrangler dev --local for Cloudflare validation
- Start fastly compute serve for Fastly validation
- Add waitForServer() helper to poll for server readiness
- Make HTTP request to validate bundle works at runtime
- Kill server processes and clean up after validation
- Return validation results object

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

Signed-off-by: Lars Trieloff <lars@trieloff.net>
Remove hostOverride and sniHostname from dynamic backend creation.
Per Fastly docs, only name, target, and useSSL are required.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

Signed-off-by: Lars Trieloff <lars@trieloff.net>
Copilot AI review requested due to automatic review settings December 5, 2025 10:59
Copilot finished reviewing on behalf of trieloff December 5, 2025 11:02
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a modern ESBuild-based bundler (EdgeESBuildBundler) to replace the webpack-based EdgeBundler for edge compute platforms (Cloudflare Workers and Fastly Compute@Edge). The new bundler offers faster build times and better platform compatibility.

Key Changes

  • New ESBuild bundler: Implements EdgeESBuildBundler with ESBuild for faster bundling, platform-specific module handling (fastly:* externals), and local validation support
  • Fastly runtime abstraction: Creates fastly-runtime.js module to centralize Fastly module imports and enable better testing via mocking
  • Enhanced platform detection: Updates edge adapter to dynamically detect Cloudflare vs Fastly environments and route requests accordingly
  • Secret store migration: Moves from Fastly Dictionaries to Secret Stores for better security and parameter management
  • Comprehensive testing: Adds extensive unit and integration tests for the new bundler with local runtime validation

Reviewed changes

Copilot reviewed 34 out of 36 changed files in this pull request and generated 14 comments.

Show a summary per file
File Description
src/EdgeESBuildBundler.js New ESBuild-based bundler with validation support
src/template/fastly-runtime.js Centralized Fastly module loading abstraction
src/template/edge-index.js Updated platform detection with async handler resolution
src/template/fastly-adapter.js Refactored to use fastly-runtime and Secret Stores
src/template/cloudflare-adapter.js Enhanced worker name extraction from hostname
src/template/polyfills/fetch.js Improved fetch polyfill with backend auto-resolution
src/template/context-logger.js Updated logger to use fastly-runtime abstraction
src/ComputeAtEdgeDeployer.js Migrated from Dictionaries to Secret Stores
src/CloudflareDeployer.js Added lowercase normalization for consistency
src/FastlyGateway.js Simplified by removing package parameter management
src/index.js Updated plugin exports to use EdgeESBuildBundler
test/build.esbuild.test.js Comprehensive ESBuild bundler tests with local validation
test/edge-integration.test.js New integration tests for both Cloudflare and Fastly
test/fastly-adapter.test.js Enhanced tests with mocking via esmock
test/fixtures/esbuild-action/* New test fixture for ESBuild bundler validation
test/fixtures/edge-action/src/index.js Enhanced with logging tests and environment detection
package.json Added esbuild, fs-extra, esmock, wrangler dependencies
.env.example New environment variable template for credentials

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +336 to +343
// Kill wrangler process
if (wranglerProcess) {
wranglerProcess.kill('SIGTERM');
}
// Clean up temporary wrangler.toml
const wranglerToml = path.join(bundleDir, 'wrangler.toml');
await fse.remove(wranglerToml).catch(() => {});
}
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The finally block attempts to kill the wrangler process, but there's no guarantee the process has time to clean up. Consider adding a small delay after kill() or waiting for the process to exit:

if (wranglerProcess) {
  wranglerProcess.kill('SIGTERM');
  // Wait a bit for graceful shutdown
  await new Promise((resolve) => setTimeout(resolve, 1000));
}

This pattern appears twice (wrangler and fastly validation), so the same improvement applies to both.

Copilot uses AI. Check for mistakes.
'-l', 'major',
'-l', 'minor',
'--bundler', 'webpack',
'--bundler', 'esbuild',
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bundler was changed from 'webpack' to 'esbuild' in a skipped test. Since this test is skipped anyway, this change may not be intentional or might cause the test to fail when it's re-enabled. Consider verifying if this change is necessary.

Copilot uses AI. Check for mistakes.
* @param {string|Request} resource - URL or Request object
* @param {object} [options] - Fetch options with cacheOverride
* @returns {Promise<Response>} Fetch response
* Wrapped fetch that supports the cacheOverride option and automatic backend resolution for Fastly
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JSDoc comment for the wrappedFetch function is incomplete. It should document the backend option and the platform-specific behavior:

/**
 * Wrapped fetch that supports the cacheOverride option and automatic backend resolution for Fastly
 * @param {string|Request} resource - URL or Request object
 * @param {object} [options] - Fetch options
 * @param {CacheOverride} [options.cacheOverride] - Cache override configuration
 * @param {string|object} [options.backend] - Fastly backend (auto-resolved from hostname if not provided)
 * @returns {Promise<Response>} Fetch response
 */
Suggested change
* Wrapped fetch that supports the cacheOverride option and automatic backend resolution for Fastly
* Wrapped fetch that supports the cacheOverride option and automatic backend resolution for Fastly.
*
* On Fastly, if the `backend` option is not provided, it is automatically resolved from the resource's hostname.
* On Cloudflare, the `cacheOverride` option is translated to Cloudflare-specific cache options.
* On other platforms, the function falls back to the standard fetch behavior.
*
* @param {string|Request} resource - URL or Request object to fetch.
* @param {object} [options] - Fetch options.
* @param {CacheOverride} [options.cacheOverride] - Cache override configuration.
* @param {string|object} [options.backend] - Fastly backend to use for the request. If not provided on Fastly, it is auto-resolved from the resource's hostname.
* @returns {Promise<Response>} Fetch response.

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,335 @@
/*
* Copyright 2024 Adobe. All rights reserved.
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The copyright year in this new file is 2024, but according to the PR metadata, it's currently December 2025. The copyright should be 2025 to match the current year.

Suggested change
* Copyright 2024 Adobe. All rights reserved.
* Copyright 2025 Adobe. All rights reserved.

Copilot uses AI. Check for mistakes.
Comment on lines +262 to +278
// eslint-disable-next-line no-await-in-loop
while (Date.now() - start < timeout) {
try {
// eslint-disable-next-line no-await-in-loop
const response = await fetch(url);
if (response.ok || response.status < 500) {
return true;
}
} catch {
// Server not ready yet, continue polling
}
// eslint-disable-next-line no-await-in-loop
await new Promise((resolve) => {
setTimeout(resolve, interval);
});
}
return false;
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The while loop uses a blocking pattern with Date.now() checks. Consider using async/await with a loop counter instead of time-based polling to make the logic clearer and avoid potential timing issues:

async waitForServer(url, timeout = 10000, interval = 500) {
  const maxAttempts = Math.floor(timeout / interval);
  for (let attempt = 0; attempt < maxAttempts; attempt++) {
    try {
      const response = await fetch(url);
      if (response.ok || response.status < 500) {
        return true;
      }
    } catch {
      // Server not ready yet, continue polling
    }
    await new Promise((resolve) => setTimeout(resolve, interval));
  }
  return false;
}

Copilot uses AI. Check for mistakes.
Headers: OriginalHeaders,
} = globalThis;

/**
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CacheOverride class is missing JSDoc documentation for the constructor, which would help users understand the parameters and options. Consider adding:

/**
 * Unified CacheOverride class that works across Fastly and Cloudflare platforms
 * @param {string|object} modeOrInit - Either a mode string ('pass', 'override', 'none') or init object
 * @param {object} [init] - Optional init object when mode is first param
 * @param {number} [init.ttl] - Time-to-live in seconds
 * @param {string} [init.cacheKey] - Custom cache key
 * @param {string} [init.surrogateKey] - Surrogate keys for cache purging (space-separated)
 */

Copilot uses AI. Check for mistakes.
Comment on lines +283 to +284
* Starts a local server, makes an HTTP request to validate, then stops the server.
* @returns {Promise<{wrangler?: boolean, fastly?: boolean}>} Validation results
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The validateBundle() method performs local validation by starting servers and making HTTP requests. This method should document:

  1. That it requires external tools (wrangler/fastly CLI) to be installed
  2. That it starts local servers on random ports
  3. That it may take significant time to complete
  4. What the return value represents

Consider adding comprehensive JSDoc:

/**
 * Validates the edge bundle using wrangler (Cloudflare) or viceroy (Fastly) if available.
 * This method performs local validation by:
 * 1. Starting a local development server (wrangler or fastly CLI)
 * 2. Making an HTTP request to validate the bundle works
 * 3. Stopping the server
 * 
 * Requirements:
 * - wrangler CLI must be installed for Cloudflare validation
 * - fastly CLI must be installed for Fastly validation
 * 
 * @returns {Promise<{wrangler?: boolean, fastly?: boolean}>} Validation results for each platform
 */
Suggested change
* Starts a local server, makes an HTTP request to validate, then stops the server.
* @returns {Promise<{wrangler?: boolean, fastly?: boolean}>} Validation results
*
* This method performs local validation by:
* 1. Starting a local development server (wrangler or fastly CLI) on a random port.
* 2. Making an HTTP request to validate that the bundle works as expected.
* 3. Stopping the server after validation.
*
* Requirements:
* - The `wrangler` CLI must be installed and available in the system PATH for Cloudflare validation.
* - The `fastly` CLI must be installed and available in the system PATH for Fastly validation.
*
* Side Effects:
* - Starts and stops local servers on random ports.
* - May use system resources and network ports temporarily.
*
* Performance:
* - This method may take significant time to complete, depending on server startup and validation.
*
* @returns {Promise<{wrangler?: boolean, fastly?: boolean}>}
* An object indicating validation results for each platform:
* - `wrangler`: true if Cloudflare validation succeeded, false if failed, undefined if not attempted.
* - `fastly`: true if Fastly validation succeeded, false if failed, undefined if not attempted.

Copilot uses AI. Check for mistakes.
Comment on lines +21 to +114
// eslint-disable-next-line no-console
console.log('=== EDGE-INDEX.JS LOADING ===');

async function detectPlatform(request) {
// eslint-disable-next-line no-console
console.log('detectPlatform called, cached:', detectedPlatform);

if (detectedPlatform) return detectedPlatform;

// eslint-disable-next-line no-console
console.log('detectPlatform: checking request.cf, request exists:', !!request);
// eslint-disable-next-line no-console
console.log('detectPlatform: request.cf =', request?.cf);

// Check for Cloudflare by testing for request.cf property
// https://developers.cloudflare.com/workers/runtime-apis/request/#incomingrequestcfproperties
if (request && request.cf) {
detectedPlatform = 'cloudflare';
// eslint-disable-next-line no-console
console.log('detected cloudflare environment via request.cf');
return detectedPlatform;
}

// eslint-disable-next-line no-console
console.log('detectPlatform: no request.cf, trying fastly:env import');

// Try Fastly by checking for fastly:env module
try {
/* eslint-disable-next-line import/no-unresolved */
await import(/* webpackIgnore: true */ 'fastly:env');
detectedPlatform = 'fastly';
// eslint-disable-next-line no-console
console.log('detected fastly environment via fastly:env import');
return detectedPlatform;
} catch (err) {
// eslint-disable-next-line no-console
console.log('detectPlatform: fastly:env import failed:', err?.message || err);
}

// eslint-disable-next-line no-console
console.log('detectPlatform: no platform detected, returning null');
return null;
}

async function getHandler(request) {
// eslint-disable-next-line no-console
console.log('getHandler called');
const platform = await detectPlatform(request);
// eslint-disable-next-line no-console
console.log('getHandler: platform detected as:', platform);

if (platform === 'cloudflare') {
// eslint-disable-next-line no-console
console.log('getHandler: returning cloudflare handler');
return handleCloudflareRequest;
}

if (platform === 'fastly') {
// eslint-disable-next-line no-console
console.log('getHandler: returning fastly handler');
return handleFastlyRequest;
}

// eslint-disable-next-line no-console
console.log('getHandler: no handler found, returning null');
return null;
}

// eslint-disable-next-line no-console
console.log('=== REGISTERING FETCH EVENT LISTENER ===');

// eslint-disable-next-line no-restricted-globals
addEventListener('fetch', (event) => {
const handler = cloudflare() || fastly();
if (typeof handler === 'function') {
event.respondWith(handler(event));
}
// eslint-disable-next-line no-console
console.log('=== FETCH EVENT RECEIVED ===');
// eslint-disable-next-line no-console
console.log('event.request.url:', event.request?.url);

event.respondWith(
getHandler(event.request).then((handler) => {
// eslint-disable-next-line no-console
console.log('getHandler resolved, handler type:', typeof handler);
if (typeof handler === 'function') {
return handler(event);
}
// eslint-disable-next-line no-console
console.log('ERROR: No handler found - Unknown platform');
return new Response('Unknown platform', { status: 500 });
}),
);
});

// eslint-disable-next-line no-console
console.log('=== EDGE-INDEX.JS FULLY LOADED ===');
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are multiple debug console.log() statements throughout this file (lines 22, 26, 31-34, 40, 45, 52-53, 56-57, 60-61, 67, 70, 74, 80, 85, 90, 95, 97, 102, 107, 114). These should either be removed for production code or guarded behind a debug flag. Consider using a debug utility or removing them:

const DEBUG = false; // or process.env.DEBUG_EDGE === 'true'
const log = DEBUG ? console.log : () => {};

Debug logging in production can impact performance and leak implementation details.

Copilot uses AI. Check for mistakes.
# Get this from: https://manage.fastly.com/account/personal/tokens
HLX_FASTLY_AUTH=your_fastly_api_token_here

# Cloudflare API token for Workers deployments
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a trailing space after "deployments" on line 8.

Suggested change
# Cloudflare API token for Workers deployments
# Cloudflare API token for Workers deployments

Copilot uses AI. Check for mistakes.
Comment on lines +82 to +105
return getSecretStore().then((SecretStore) => {
if (!SecretStore) {
return undefined;
}
const url = target.get('_package');
const token = target.get('_token');
// console.log(`Getting secrets from ${url} with ${token}`);
return fetch(url, {
backend: 'gateway',
headers: {
authorization: `Bearer ${token}`,
},
}).then((response) => {
if (response.ok) {
// console.log('response is ok...');
return response.text().then((json) => {
// console.log('json received: ' + json);
packageParams = JSON.parse(json);
return packageParams[prop];
}).catch((error) => {
console.error(`Unable to parse JSON: ${error.message}`);
});
// Try action_secrets first (action-specific params - highest priority)
const actionSecrets = new SecretStore('action_secrets');
return actionSecrets.get(prop).then((secret) => {
if (secret) {
return secret.plaintext();
}
console.error(`HTTP status is not ok: ${response.status}`);
return undefined;
}).catch((err) => {
console.error(`Unable to fetch parames: ${err.message}`);
// Try package_secrets next (package-wide params)
const packageSecrets = new SecretStore('package_secrets');
return packageSecrets.get(prop).then((pkgSecret) => {
if (pkgSecret) {
return pkgSecret.plaintext();
}
return undefined;
});
});
}
}).catch((err) => {
// eslint-disable-next-line no-console
console.error(`Error accessing secrets for ${prop}: ${err.message}`);
return undefined;
});
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Promise-based get trap in the env Proxy returns a Promise, which means accessing context.env.FOO will return a Promise, not the actual value. This requires consumers to use await context.env.FOO everywhere. While this is shown in the test fixtures, it could be error-prone. Consider documenting this behavior clearly in code comments:

// Load SecretStore dynamically and access secrets
// NOTE: All env property accesses return Promises and must be awaited:
//   const value = await context.env.FOO;
return getSecretStore().then((SecretStore) => {

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants