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

Make createCors({ origins }) to accept function #157

Merged
merged 9 commits into from Nov 25, 2023

Conversation

leaysgur
Copy link
Contributor

The createCors({ origins }) now accepts functions as well.

This makes it easy to support dynamically changing subdomains, as in the Cloudflare Pages branch preview.

createCors({
  origins: (origin) => {
    // Root
    if (origin === "https://our-pages.example.com") return true;
    // Commit preview, branch preview
    if (origin.endsWith(".our-pages.example.com") return true;
    // Another service may want to access
    if (origin === "https://another-our-pages.example.com") return true;

    return false;
  },
})

While it's possible to achieve this by accepting RegExp, but functions are more flexible and often more readable.
Of course RegExp can be used by (origin) => re.test(origin).

For backwards compatibility, the signature is a union with string[].
(IMO: It's fine with just functions.)

What do you think?

@leaysgur
Copy link
Contributor Author

One other thing I noticed. 👀

Currently, createCors.spec.ts does not pass on the v4.x branch.
This is because new Response(null, { status: 101 }) will result in a RangeError.

The error seems to occur when using v18 or later Node.js(w/ undici), and will eventually occur in the near future even when using isomorphic-fetch with earlier versions.

node-fetch/node-fetch#1685

But this code does not throw acutualy in Edge workers like Cloudflare Workers... 😅

@kwhitley
Copy link
Owner

One other thing I noticed. 👀

Currently, createCors.spec.ts does not pass on the v4.x branch. This is because new Response(null, { status: 101 }) will result in a RangeError.

The error seems to occur when using v18 or later Node.js(w/ undici), and will eventually occur in the near future even when using isomorphic-fetch with earlier versions.

node-fetch/node-fetch#1685

But this code does not throw acutualy in Edge workers like Cloudflare Workers... 😅

Great catch, thanks for the investigation! Since I'm trying to launch v4 in the next day or two (if enough folks test out the new typings), I'll def want to sort this ASAP.

@kwhitley
Copy link
Owner

kwhitley commented May 21, 2023

Yeah, @leader22 - this looks clean, and adds very little size for the added support. Love it!

Would you mind making a pass at updating the v4.x docs?

CORS page
https://github.com/kwhitley/itty.dev/blob/main/src/routes/itty-router/cors/%2Bpage.md

API page (at the createCors entry)
https://github.com/kwhitley/itty.dev/blob/main/src/routes/itty-router/api/%2Bpage.md

Thanks!

@kwhitley kwhitley added new feature New feature or request PENDING RELEASE Final stages before release! :D labels May 21, 2023
@leaysgur
Copy link
Contributor Author

@kwhitley Thanks for the review!

updating the v4.x docs?

Like this? 👉🏻 kwhitley/itty.dev#1

@kwhitley
Copy link
Owner

Exactly! I'm not feeling great at the moment, so this will wait till tomorrow or so, but I think this is a great addition, and def adds to the flexibility (one of the core principles of itty)!

@kwhitley
Copy link
Owner

@leader22 - createCors underwent a GPT refactor to save bytes, causing a collision with this PR.

This one I'm not super worried about timing on, as it's a minor release/feature add on core - so happy to add it after v4.x goes out.

@leaysgur
Copy link
Contributor Author

leaysgur commented May 23, 2023

I see.

In that sense, I think I can resolve the conflict now, but should I wait for the v4.x release? 👀


And now fixed, let me know if there is anything else I can help. 😉

@kwhitley
Copy link
Owner

Honestly, I'd say we wait at this point - that way you get some dedicated credit in the changelog, haha. Right now, there are so many changes, with help from so many folks, that a lot of well-deserved credit will already be lost! 😭

@kwhitley
Copy link
Owner

I'd say as soon as 4.x goes out and we patch any immediate issues (hopefully none), we roll this out in 4.1.0

@leaysgur
Copy link
Contributor Author

👍🏻 , looking forward to it~!

@kwhitley kwhitley merged commit 80f89b2 into kwhitley:v4.x Nov 25, 2023
4 checks passed
@leaysgur leaysgur deleted the v4.x branch November 25, 2023 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request PENDING RELEASE Final stages before release! :D
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants