fix: bundle dynamic API routes correctly with split-api-routes #2154
Conversation
✅ Deploy Preview for netlify-plugin-nextjs-next-auth-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for netlify-plugin-nextjs-export-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for next-i18next-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for netlify-plugin-nextjs-static-root-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for netlify-plugin-nextjs-nx-monorepo-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for next-plugin-edge-middleware ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for next-plugin-canary ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for nextjs-plugin-custom-routes-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for netlify-plugin-nextjs-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
Here's the failing test: https://github.com/netlify/next-runtime/actions/runs/5176735315/jobs/9325929677#step:10:236 |
|
d19f422 should make it green 🟢 |
There was a problem hiding this comment.
The fix looks good, but I'm trying to understand how our existing tests didn't fail as we have other dynamic routes. Is the API route splitting only enabled on the default demo site?
Also, it'd be good to test with and without the split API routes feature flag as it's not rolled out to everyone yet.
There was a problem hiding this comment.
The fix looks good, but I'm trying to understand how our existing tests didn't fail as we have other dynamic routes. Is the API route splitting only enabled on the default demo site?
Also, it'd be good to test with and without the split API routes feature flag as it's not rolled out to everyone yet.
The bug only exists for API Routes, not for SSR routes, since we're currently only using the new bundling mechanism for API Routes. I don't think we tested any other dynamic API Routes before, or else we'd have caught this bug earlier.
It's enabled in 4 demo sites, which I think are the sites that we're running e2e tests against: https://github.com/search?q=repo:netlify/next-runtime+NEXT_SPLIT_API_ROUTES&type=code
With the way our e2e tests work, I don't see how we can do that. I think that's out-of-scope for this PR as well. I'd prefer that we get the split-api-routes work done quickly, and remove the featureflag before it becomes long-lived enough to warrant additional test cases. |
| * Transforms `/api/shows/[id].js` into `/api/shows/*id*.js`, | ||
| * so that the file `[id].js` is matched correctly. | ||
| */ | ||
| const escapeGlob = (path: string) => path.replace(/\[/g, '*').replace(/]/g, '*') |
There was a problem hiding this comment.
i think this is going to match unwanted files, e.g. grid.js
is it possible to escape the path, e.g. \[id\].js ?
There was a problem hiding this comment.
I tried escaping the path, and glob patterns do allow escaping the brackets - but I couldn't get that to work properly with how ZISI interprets the globs. Maybe we could open a follow-up issue to investigate this.
Yes, this matches too many files - but I don't think that it'll be a big problem in practice.
There was a problem hiding this comment.
This seems to have been fixed in v9 of glob: isaacs/node-glob#277 (comment)
Would it be reasonable to wait for a new ZISI release updating that dependency? If it would take too long than we could do this for now and create a follow-up
There was a problem hiding this comment.
Good to know it's fixed in v9! I'd prefer the follow-up. Let's open an issue for this.
There was a problem hiding this comment.
Description
We discovered that dynamic API routes, e.g.
/api/shows/[id].js, aren't bundled correctly. This is likely due to the square brackets being interpreted as special glob patterns, resulting in the underlying files not being included correctly.This PR adds an e2e test that demonstrates the bug, and then fixes it by processing the glob patterns before passing them to the build process.
Documentation
Tests
You can test this change yourself like so:
Relevant links (GitHub issues, etc.) or a picture of cute animal
https://github.com/netlify/pod-dev-foundations/issues/516