Skip to content

Conversation

@magnusdahlstrand
Copy link
Contributor

@magnusdahlstrand magnusdahlstrand commented Jan 23, 2024

Relates to #4265.

What this PR solves / how to test:
createMetadataObject previously only received a single logging function set to warn. This PR updates it to receive a full Logger object, and updates it to use the various levels correctly.

Previously messages such as [WARNING] Parsed 2 valid header rules. were output to the console, whereas with this fix they're no longer warnings, and therefore can be muted with --log-level=info.

Additionally, wrangler/src/miniflare-cli/assets.ts was defining its own Logger interface but is now loading the interface from the logger.ts file.

Author has addressed the following:

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.

@magnusdahlstrand magnusdahlstrand requested review from a team as code owners January 23, 2024 15:33
@changeset-bot
Copy link

changeset-bot bot commented Jan 23, 2024

🦋 Changeset detected

Latest commit: c3a2396

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

This PR includes changesets to release 2 packages
Name Type
@cloudflare/pages-shared Patch
wrangler 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

@magnusdahlstrand magnusdahlstrand force-pushed the fix-header-parsing-verbosity branch from 7b76900 to c1584c0 Compare January 23, 2024 15:55
@github-actions
Copy link
Contributor

github-actions bot commented Jan 23, 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/7697441483/npm-package-wrangler-4819

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7697441483/npm-package-wrangler-4819 dev path/to/script.js
Additional artifacts:
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7697441483/npm-package-create-cloudflare-4819 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7697441483/npm-package-miniflare-4819
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/7697441483/npm-package-cloudflare-pages-shared-4819

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.20231218.4
workerd 1.20231218.0 1.20231218.0
workerd --version 1.20231218.0 2023-12-18

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

@codecov
Copy link

codecov bot commented Jan 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c3e410c) 71.47% compared to head (c3a2396) 70.71%.
Report is 28 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4819      +/-   ##
==========================================
- Coverage   71.47%   70.71%   -0.76%     
==========================================
  Files         288      291       +3     
  Lines       14831    15164     +333     
  Branches     3729     3859     +130     
==========================================
+ Hits        10600    10723     +123     
- Misses       4231     4441     +210     

see 19 files with indirect coverage changes

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

LGTM - can you fix up the changeset to be more clear what is changing?

Update changeset to be more user focussed

Co-authored-by: Pete Bacon Darwin <[email protected]>
@magnusdahlstrand
Copy link
Contributor Author

Thanks @petebacondarwin for your input on the changeset, let me know if anything else is needed from my side!

@lrapoport-cf
Copy link
Contributor

can someone from @cloudflare/pages please review this as well? cc @GregBrimble @jahands @WalshyDev @jrf0110 @CarmenPopoviciu

@lrapoport-cf lrapoport-cf added pages Relating to Pages awaiting Cloudflare response Awaiting response from workers-sdk maintainer team labels Jan 29, 2024
Copy link
Contributor

@GregBrimble GregBrimble left a comment

Choose a reason for hiding this comment

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

LGTM!

@petebacondarwin petebacondarwin merged commit 6a4cb8c into cloudflare:main Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting Cloudflare response Awaiting response from workers-sdk maintainer team pages Relating to Pages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants