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

fix(vitest-pool-workers) Fix support for query params with repeated keys #7668

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

romeupalos
Copy link
Contributor

Fixes #7667

Support interception of URLs with repeated key/name in its query params. e.g., /index.html?a=1&a=2.


  • Tests
    • TODO (before merge)
    • Tests included
    • Tests not necessary because:
  • E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required
    • Not required because:
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Documentation not necessary because: this is a bug fix

@romeupalos romeupalos requested a review from a team as a code owner January 5, 2025 10:03
Copy link

changeset-bot bot commented Jan 5, 2025

🦋 Changeset detected

Latest commit: 260270b

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

This PR includes changesets to release 1 package
Name Type
@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

@romeupalos
Copy link
Contributor Author

The change is inspired by undici code itself, where they concatenate the pathname with the search attribute.
https://github.com/nodejs/undici/blob/main/index.js#L105

Copy link
Member

@edmundhung edmundhung left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! This looks great to me. I will run the e2e tests for a sanity check and merge it once everything passes. 👍🏼

@edmundhung edmundhung added the e2e Run wrangler e2e tests on a PR label Jan 6, 2025
@edmundhung edmundhung removed the e2e Run wrangler e2e tests on a PR label Jan 6, 2025
@edmundhung edmundhung merged commit 94f650e into cloudflare:main Jan 6, 2025
37 of 42 checks passed
@workers-devprod workers-devprod added the contribution [Holopin] Recognizes an open-source contribution, big or small label Jan 6, 2025
Copy link

holopin-bot bot commented Jan 6, 2025

Congratulations @romeupalos, the maintainer of this repository has issued you a holobyte! Here it is: https://holopin.io/holobyte/cm5l7w3j406680cl5g9otx1zn

This badge can only be claimed by you, so make sure that your GitHub account is linked to your Holopin account. You can manage those preferences here: https://holopin.io/account.
Or if you're new to Holopin, you can simply sign up with GitHub, which will do the trick!

@workers-devprod workers-devprod mentioned this pull request Jan 6, 2025
@Skye-31
Copy link
Contributor

Skye-31 commented Jan 6, 2025

I think this breaks anyone who's currently doing matching against single query params

@edmundhung
Copy link
Member

@Skye-31 Do you mean intercepting with the query property? I added a test here and it seems to work fine?

@Skye-31
Copy link
Contributor

Skye-31 commented Jan 6, 2025

@Skye-31 Do you mean intercepting with the query property? I added a test here and it seems to work fine?

Yes, I was concerned that would break - if there's a test to ensure it doesn't, I'm happy!

@edmundhung
Copy link
Member

Makes sense 👍🏼 Added tests on #7682

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution [Holopin] Recognizes an open-source contribution, big or small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 BUG: vitest-pool-workers fail to mock URLs that contains repeated keys in the query section
4 participants