kwhitley / itty-router

A little router.
MIT License
1.7k stars 77 forks source link

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

Closed leaysgur closed 7 months ago

leaysgur commented 1 year ago

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 commented 1 year ago

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.

https://github.com/node-fetch/node-fetch/issues/1685

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

kwhitley commented 1 year ago

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 commented 1 year ago

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!

leaysgur commented 1 year ago

@kwhitley Thanks for the review!

updating the v4.x docs?

Like this? 👉🏻 https://github.com/kwhitley/itty.dev/pull/1

kwhitley commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

👍🏻 , looking forward to it~!