fix: make sure that fetch cache sets are properly awaited

[dario-piotrowicz]

Next.js does not await promises that update the incremental cache for fetch requests,
that is needed in our runtime otherwise the cache updates get lost, so this change
makes sure that the promise is properly awaited via waitUntil

---

resolves #328

[changeset-bot]

🦋 Changeset detected

Latest commit: 107b285

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

This PR includes changesets to release 1 package

Name	Type
@opennextjs/cloudflare	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

[pkg-pr-new]

Open in Stackblitz

pnpm add https://pkg.pr.new/@opennextjs/cloudflare@364

commit: 107b285

[vicb]

Added some comments.
Testing unminified is the most important

[vicb]

@dario-piotrowicz maybe you try something like:

rule:
  kind: expression_statement
  all:
    - has: { pattern: "$$$_.then($$$_)", stopBy: end }
    - has: { pattern: "Buffer.from", stopBy: end }
    - has: { pattern: "$_.toString('base64')", stopBy: end }
    - has: { pattern: "$_.CachedRouteKind.FETCH", stopBy: end }

I have only tried on minified code but it should be possible to figure out something that works everywhere

[vicb]

rule:
  kind: expression_statement
  all:
    - has: { pattern: "$$$.then($$$)", stopBy: end }
    - has: { pattern: "Buffer.from", stopBy: end }
    - has: { pattern: "$_.toString('base64')", stopBy: end }
    - any:
        - has: { pattern: "CachedRouteKind.FETCH", stopBy: end }
        - has: { pattern: "$_.CachedRouteKind.FETCH", stopBy: end }
    - has: { pattern: "$$$.finally($$$)", stopBy: end }

[dario-piotrowicz]

rule:
  kind: expression_statement
  all:
    - has: { pattern: "$$$.then($$$)", stopBy: end }
    - has: { pattern: "Buffer.from", stopBy: end }
    - has: { pattern: "$_.toString('base64')", stopBy: end }
    - any:
        - has: { pattern: "CachedRouteKind.FETCH", stopBy: end }
        - has: { pattern: "$_.CachedRouteKind.FETCH", stopBy: end }
    - has: { pattern: "$$$.finally($$$)", stopBy: end }

this doesn't work, if you want you can check that in the playground I've been using, here's the link to it: https://pastebin.mozilla.org/eEiLmsaF

with my rule there are three matches, with yours 4 incorrect ones (I've also tried your rule in the adapter and it didn't result in a valid patch)

[vicb]

Nice work!

I added a comment inline, but LGTM 🚀

[dario-piotrowicz]

One last thing before merging!

The usage of getCrossPlatformPathRegex seems to cause the chunk file not be be patched, I temporarily reverted that (5e97964) I'm looking into understanding why 👀

[conico974]

@dario-piotrowicz There might be an issue with getCrossPlatformPathRegex, i'm wondering if it has something to do with String.raw ?

[vicb]

@dario-piotrowicz There might be an issue with getCrossPlatformPathRegex, i'm wondering if it has something to do with String.raw ?

Looking at .source might help:

[dario-piotrowicz]

@dario-piotrowicz There might be an issue with getCrossPlatformPathRegex, i'm wondering if it has something to do with String.raw ?

No I've been debugging it and I'm pretty sure it has to do with the g flag that makes the regex side effectful (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/lastIndex), that causes it to go out of whack and not match things properly

this fixes the issue:

But ideally I think we should add an option to getCrossPlatformPathRegex to make the g flag opt-in (or opt-out) (or just remove it entirely)

[conico974]

https://github.com/opennextjs/opennextjs-aws/blob/b59027a5899d25dd5263d1a272b33ec23fb683d3/packages/open-next/src/utils/regex.ts#L20
You should be able to change the flags (especially with escape false)

[vicb]

Maybe we should switch to str.match(regex) instead.

I asked on the PR if we really need to g the regexs and say "it doesn't harm", that was poorly anticipating :)

[dario-piotrowicz]

https://github.com/opennextjs/opennextjs-aws/blob/b59027a5899d25dd5263d1a272b33ec23fb683d3/packages/open-next/src/utils/regex.ts#L20 You should be able to change the flags (especially with escape false)

oops, I missed that the flags were already configurable! 😅👍

[dario-piotrowicz]

Maybe we should switch to str.match(regex) instead.

good call! let's go with that 😄

[dario-piotrowicz]

@vicb, @conico974 the PR is ready to merge please let me know if we're all happy with it 🙂

[conico974]

LGTM
Nice work on this one

[vicb]

Awesome, thanks for your work on this PR 🎉