-
-
Notifications
You must be signed in to change notification settings - Fork 78
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
Conversation
One other thing I noticed. 👀 Currently, The error seems to occur when using v18 or later Node.js(w/ But this code does not |
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. |
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 API page (at the createCors entry) Thanks! |
@kwhitley Thanks for the review!
Like this? 👉🏻 kwhitley/itty.dev#1 |
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)! |
@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. |
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. 😉 |
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! 😭 |
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 |
👍🏻 , looking forward to it~! |
The
createCors({ origins })
now accepts functions as well.This makes it easy to support dynamically changing subdomains, as in the Cloudflare Pages branch preview.
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?