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

feat (pages): Proxying (200) in _redirects #2708

Merged
merged 4 commits into from
Feb 16, 2023

Conversation

Skye-31
Copy link
Contributor

@Skye-31 Skye-31 commented Feb 10, 2023

What this PR solves / how to test:
This PR enables Pages _redirects to support proxying to files on a different url, for example pointing /users/123 to /users/id.html, to enable a more dynamic functionality for static sites.

Associated docs issues/PR:

Author has included the following, where applicable:

  • Tests
  • Changeset

Reviewer has performed the following, where 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

@changeset-bot
Copy link

changeset-bot bot commented Feb 10, 2023

🦋 Changeset detected

Latest commit: e04b0e1

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 Minor
wrangler Minor

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

@github-actions
Copy link
Contributor

github-actions bot commented Feb 10, 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/runs/4193296716/npm-package-wrangler-2708

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

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

Or you can use npx with this latest build directly:

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

@Skye-31 Skye-31 force-pushed the skye/pages-_redirects-proxying branch from 70e0052 to c11c6b9 Compare February 10, 2023 12:28
@GregBrimble GregBrimble marked this pull request as ready for review February 10, 2023 16:56
@GregBrimble GregBrimble requested review from a team as code owners February 10, 2023 16:56
@GregBrimble
Copy link
Member

Ah, sorry! Wrong button. Meant to approve your workflows and accidentally marked this PR as ready for review. Feel free to put it back, if you can :)

@Skye-31 Skye-31 marked this pull request as draft February 10, 2023 20:20
@Skye-31
Copy link
Contributor Author

Skye-31 commented Feb 10, 2023

No worries 😄
Just keeping it as draft until I get around to the docs on the weekend, I belive it's good to go otherwise!

@Skye-31
Copy link
Contributor Author

Skye-31 commented Feb 11, 2023

Docs pull made

@Skye-31 Skye-31 marked this pull request as ready for review February 11, 2023 11:12
@Skye-31 Skye-31 force-pushed the skye/pages-_redirects-proxying branch from 5eca44d to 3361d91 Compare February 14, 2023 13:01
Copy link
Member

@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.

Amazing stuff. Thank you for your contribution! Only thing I'd like to look at is reusing the existing validateUrl() function. Everything else looks perfect :)

@Skye-31 Skye-31 force-pushed the skye/pages-_redirects-proxying branch from 3361d91 to 524f6d3 Compare February 15, 2023 14:34
Copy link
Member

@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! Thanks again for the contribution! I'll leave this up for a day just to allow others to comment if others have opinions, and then I'll merge.

It'll be available in wrangler@beta pages dev immediately, and then @cloudflare/pages-shared and wrangler@latest when we next cut a release. We'll pull in these changes in production shortly afterwards and then do a release. Probably ~mid-next-week-ish by the time that it's live on Cloudflare Pages.

@codecov
Copy link

codecov bot commented Feb 15, 2023

Codecov Report

Merging #2708 (54a4e19) into main (0f29093) will increase coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 54a4e19 differs from pull request most recent head e04b0e1. Consider uploading reports for the commit e04b0e1 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2708      +/-   ##
==========================================
+ Coverage   74.03%   74.06%   +0.02%     
==========================================
  Files         166      166              
  Lines       10154    10154              
  Branches     2702     2702              
==========================================
+ Hits         7518     7521       +3     
+ Misses       2636     2633       -3     
Impacted Files Coverage Δ
packages/wrangler/src/git-client.ts 43.75% <0.00%> (+4.16%) ⬆️
...ackages/wrangler/src/__tests__/helpers/mock-bin.ts 100.00% <0.00%> (+5.26%) ⬆️

@Skye-31 Skye-31 force-pushed the skye/pages-_redirects-proxying branch from 54a4e19 to e04b0e1 Compare February 16, 2023 11:03
@GregBrimble GregBrimble merged commit b3346cf into cloudflare:main Feb 16, 2023
@github-actions github-actions bot mentioned this pull request Feb 16, 2023
@Skye-31 Skye-31 deleted the skye/pages-_redirects-proxying branch February 16, 2023 13:32
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.

None yet

3 participants