perf(cloudflare): check for asset url

[Hebilicious]

Remove KV access when an API route is called.

🔗 Linked issue

Fix https://github.com/unjs/nitro/issues/1001 Resolves https://github.com/unjs/nitro/issues/1235

❓ Type of change

• [ ] 📖 Documentation (updates to the documentation or readme)
• [x] 🐞 Bug fix (a non-breaking change that fixes an issue)
• [x] 👌 Enhancement (improving an existing functionality like performance)
• [ ] ✨ New feature (a non-breaking change that adds functionality)
• [ ] ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

📝 Checklist

• [x] I have linked an issue or discussion.
• [ ] I have updated the documentation accordingly.

[Hebilicious]

This was useful to debug KV :

  const list = await env.__STATIC_CONTENT.list();
  console.log("GET ASSET FROM KV", list.keys.map((k) => k.name), { publicAssetBases });

Should we add e2e tests ? Especially Nuxt ones. IE deploy nuxt project with nitro + wrangler and hit the endpoints.

[pi0]

This is a nice enhancenment! I think we can use existing isPublicAssetURL(id) utility also to check both both direct ids and public base URLs. Any reason you haven't use that?

[Hebilicious]

This is a nice enhancenment! I think we can use existing isPublicAssetURL(id) utility also to check both both direct ids and public base URLs. Any reason you haven't use that?

I first tried to use isPublicAssetURL, but during my testing I've noticed that it wasn't matching properly for some of the routes, so I added the extra check. Adding to isPublicAssetURL broke some other provider test iirc.

But maybe isInKV should be in the virtual file generated by the rollup plugin ?

[pi0]

Can you please explain more what routes are not being matched?

[Hebilicious]

Can you please explain more what routes are not being matched?

Sure ! For the Cloudflare Module for example :

Running the tests with isInPublicAssets

Running the tests with isInKv

Mabye my assumption is wrong, but because /api/hey/index.html exist, I assumed that calling /api/hey/index should hit the KV store and return the index.html

This is what's in the KV store before getAssetFromKV is called :

GET ASSET FROM KV [
  '$__MINIFLARE_SITES__$/api%2Fhello',
  '$__MINIFLARE_SITES__$/api%2Fhey%2Findex.html',
  '$__MINIFLARE_SITES__$/api%2Fparam%2Ffoo.json%2Findex.html',
  '$__MINIFLARE_SITES__$/api%2Fparam%2Fprerender1%2Findex.html',
  '$__MINIFLARE_SITES__$/api%2Fparam%2Fprerender3%2Findex.html',
  '$__MINIFLARE_SITES__$/api%2Fparam%2Fprerender4%2Findex.html',
  '$__MINIFLARE_SITES__$/build%2Ftest.txt',
  '$__MINIFLARE_SITES__$/favicon.ico',
  '$__MINIFLARE_SITES__$/icon.png',
  '$__MINIFLARE_SITES__$/prerender%2Findex.html',
  '$__MINIFLARE_SITES__$/prerender%2Findex.html.br',
  '$__MINIFLARE_SITES__$/prerender%2Findex.html.gz'
] 

[codecov[bot]]

Codecov Report

Merging #1236 (2e14d6c) into main (414f5f5) will increase coverage by 0.30%. The diff coverage is n/a.

:exclamation: Current head 2e14d6c differs from pull request most recent head 3e8d3e2. Consider uploading reports for the commit 3e8d3e2 to get more accurate results

@@            Coverage Diff             @@
##             main    #1236      +/-   ##
==========================================
+ Coverage   76.58%   76.89%   +0.30%     
==========================================
  Files          71       68       -3     
  Lines        7223     6829     -394     
  Branches      718      693      -25     
==========================================
- Hits         5532     5251     -281     
+ Misses       1690     1577     -113     
  Partials        1        1              

see 23 files with indirect coverage changes

[pi0]

Good point about also checking .html suffixes. Since we are checking for public asset existence in other presets, it would be much better if we could merge isInKV functionality into isPublicAssetURL. Otherwise seems really good improvements 💯

[Hebilicious]

Good point about also checking .html suffixes. Since we are checking for public asset existence in other presets, it would be much better if we could merge isInKV functionality into isPublicAssetURL. Otherwise seems really good improvements 💯

This was done in the later commits. Reminder to review again 🙏🏽

[pi0]

Thanks for changes. As an update, i still need to revisit this issue while it is important fix and i value your efforts, i am still not convinced why we are introducing a new util like this so need to check it more in depth (i might be wrong as well!)

[Hebilicious]

Thanks for changes. As an update, i still need to revisit this issue while it is important fix and i value your efforts, i am still not convinced why we are introducing a new util like this so need to check it more in depth (i might be wrong as well!)

Now that look more at it, this PR is more of a fix than a perf, as currently there's a bug where some matched assets aren't returned by KV, and some non existing assets are being accessed.

[zsilbi]

This would be a very nice improvement! I found this as I was investigating why my noop endpoint latency is so unpredictable. (bottom)

[pi0]

With workers static assets we won't need to depend on binding anymore.