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

[pages] Update pages functions build to include config #5353

Merged
merged 13 commits into from
Mar 25, 2024

Conversation

penalosa
Copy link
Contributor

@penalosa penalosa commented Mar 22, 2024

What this PR solves / how to test

Updates wrangler pages functions build to support including values from wrangler.toml in the Functions upload. Additionally, support two properties that will be set in CI—--build-metadata-path and --project-directory.

Fixes DEVX-1177 & DEVX-1175

Author has addressed the following

Copy link

changeset-bot bot commented Mar 22, 2024

🦋 Changeset detected

Latest commit: a2541ee

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

Copy link
Contributor

github-actions bot commented Mar 22, 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/8421253010/npm-package-wrangler-5353

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8421253010/npm-package-wrangler-5353 dev path/to/script.js
Additional artifacts:
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8421253010/npm-package-create-cloudflare-5353 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8421253010/npm-package-cloudflare-kv-asset-handler-5353
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8421253010/npm-package-miniflare-5353
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8421253010/npm-package-cloudflare-pages-shared-5353
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8421253010/npm-package-cloudflare-vitest-pool-workers-5353

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


wrangler@3.37.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20240320.0
workerd 1.20240320.1 1.20240320.1
workerd --version 1.20240320.1 2024-03-20

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

@penalosa penalosa force-pushed the penalosa/pages-config-download branch from 3af8b05 to b5deb07 Compare March 22, 2024 15:28
Base automatically changed from penalosa/pages-config-download to main March 25, 2024 09:40
@petebacondarwin petebacondarwin added the ldw-exempt PRs marked with this label can be included in releases during LDW. label Mar 25, 2024
@penalosa penalosa marked this pull request as ready for review March 25, 2024 10:04
@penalosa penalosa requested review from a team as code owners March 25, 2024 10:04
Copy link

codecov bot commented Mar 25, 2024

Codecov Report

Attention: Patch coverage is 92.72727% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 72.45%. Comparing base (7d160c7) to head (a2541ee).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5353      +/-   ##
==========================================
+ Coverage   72.29%   72.45%   +0.16%     
==========================================
  Files         318      318              
  Lines       16458    16505      +47     
  Branches     4201     4230      +29     
==========================================
+ Hits        11898    11959      +61     
+ Misses       4560     4546      -14     
Files Coverage Δ
...ler/src/api/pages/create-worker-bundle-contents.ts 100.00% <100.00%> (ø)
packages/wrangler/src/api/pages/deploy.tsx 87.12% <ø> (ø)
packages/wrangler/src/pages/build.ts 70.79% <86.20%> (+3.38%) ⬆️

... and 9 files with indirect coverage changes

Copy link
Contributor

@mrbbot mrbbot left a comment

Choose a reason for hiding this comment

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

Looks good! ✅ Some very minor comments. 👍

fixtures/pages-functions-app/wrangler.toml Outdated Show resolved Hide resolved
packages/wrangler/src/pages/build.ts Outdated Show resolved Hide resolved
packages/wrangler/src/pages/build.ts Outdated Show resolved Hide resolved
packages/wrangler/src/api/pages/deploy.tsx Show resolved Hide resolved
.changeset/swift-pears-carry.md Outdated Show resolved Hide resolved
Comment on lines 21 to 23
const metadata = JSON.parse(workerBundleFormData.get("metadata") as string);

/**
* Pages doesn't need the metadata bindings returned by
* `createWorkerBundleFormData`. Let's strip them out and return only
* the contents we need
*/
workerBundleFormData.set(
"metadata",
JSON.stringify({ main_module: metadata.main_module })
);
workerBundleFormData.set("metadata", JSON.stringify(metadata));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these lines are no longer needed. They were only here because we were trying to strip out the bindings from the metadata.

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact perhaps this whole function is redundant now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've modified this function to strip out the empty bindings key from the metadata if no config has been found

packages/wrangler/src/api/pages/deploy.tsx Show resolved Hide resolved
packages/wrangler/src/pages/build.ts Show resolved Hide resolved
fixtures/pages-functions-app/wrangler.toml Outdated Show resolved Hide resolved
fixtures/pages-functions-app/wrangler.toml Outdated Show resolved Hide resolved
fixtures/pages-functions-app/wrangler.toml Outdated Show resolved Hide resolved
fixtures/pages-functions-app/wrangler.toml Outdated Show resolved Hide resolved
@petebacondarwin
Copy link
Contributor

According to the JIRA ticket we also need to support a --pages-environment=production|preview arg?

@penalosa
Copy link
Contributor Author

According to the JIRA ticket we also need to support a --pages-environment=production|preview arg?

I've updated the JIRA ticket—for consistency with build-env this is now passed as an environment variable (PAGES_ENVIRONMENT).

@penalosa penalosa merged commit 3be826f into main Mar 25, 2024
17 checks passed
@penalosa penalosa deleted the penalosa/functions-build branch March 25, 2024 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ldw-exempt PRs marked with this label can be included in releases during LDW.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants