feat: split up API Routes + use .nft.json files to make builds fast #2058
Conversation
Brings back the functionality that was reverted in #1731, but under a flag. This will be utterly slow in building, so let's try to speed that up in the next step!
✅ 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-plugin-edge-middleware 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-i18next-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ 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 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. |
github-actions
bot
added
the
type: feature
|
Is there a plan to compare build times? |
|
Ideally, i'd like to do a phased rollout where I enable this for a portion of Starter accounts, and compare build times, yes. Would be great if you could point me towards how we do phased rollouts with the Next runtime :) |
|
I feel like i'm making good progress! I'm successfully deploying individual functions, and it's leveraging the |
|
It's ok, the runtime always uses a phased rollout. What I meant though was testing it yourself by generating several API routes and checking how the build time compares. |
|
Alright, time for a first check-in! I've been testing with
https://64428a02aaf0226f07e85492--nextjs-split-routes.netlify.app/ is a working deployment with this. The files-to-be-included are identified using the There's a couple of things that I'd like to address before we can move forward with this:
Before I address all of that, a rough review would be good though to know if i'm missing something crucial. @orinokai could you take a rough look at this PR and check the approach? It looks fine to me, but I'm missing a bunch of context on this. A dime for your review :D |
| const traceNextServer = async (publish: string): Promise<string[]> => { | ||
| const nextServerDeps = await getDependenciesOfFile(join(publish, 'next-server.js')) | ||
|
|
||
| // during testing, i've seen `next-server` contain only one line. |
There was a problem hiding this comment.
This feels like a weird check?
What if Next.js itself changes the file and the .nft file changes correctly and this arbitrary threshold is triggered? What is the goal of this check?
There was a problem hiding this comment.
I've explained that here: #2058 (comment)
Rephrasing, to add more context: I've seen instances locally where the .nft file contained less than 10 lines, but those were definitely invalid ones, and they resulted in broken bundles. I don't see a case where the .nft file correctly contains less than 10 lines. I'm unsure if the instances i've seen locally are due to not-so-common setup where the plugin is pulled in with npm link. If this happened in prod, it'd result in deployments that break at runtime. To prevent this, i'm adding this build-time check. I'll be closely monitoring it during the rollout. Once I'm certain this doesn't occur in production, I'll remove the check.
| throw new Error("next-server.js.nft.json didn't contain all dependencies.") | ||
| } | ||
|
|
||
| const filtered = nextServerDeps.filter((f) => { |
There was a problem hiding this comment.
This filter list could be its own array of strings in a const and then we could add more stuff if necessary more easily?
There was a problem hiding this comment.
I could imagine that in the future, there's different kinds of patterns than .endsWith that we want to match against, so having this as a filter function is the most versatile. Having said that, I'm fine with replacing this with an array of strings for now, if you prefer that.
| if (headers['x-prerender-revalidate'] && this.netlifyConfig.revalidateToken) { | ||
| // handle on-demand revalidation by purging the ODB cache | ||
| await this.netlifyRevalidate(url) | ||
|
|
||
| res = res as unknown as BaseNextResponse | ||
| res.statusCode = 200 | ||
| res.setHeader('x-nextjs-cache', 'REVALIDATED') | ||
| res.send() | ||
| } else { | ||
| await handler(req, res, parsedUrl) |
There was a problem hiding this comment.
This feels dangerous to me - this change apply to everyone and not just folks opted into feature flag.
I don't have a lot of context on inner workings of refresh hooks (that's @orinokai expertise), so it might be fine not to exec handler but maybe we should consider using this method only when we are splitting api routes to limit potential blast radius?
I assume that this stuff is related to #2058 (comment) and calling it result in MODULE_NOT_FOUND errors you mentioned in earlier comments?
There was a problem hiding this comment.
Your assumption is right. I agree this feels dangerous - i'll see if there's an easy solution to subjecting this to the featureflag.
There was a problem hiding this comment.
put it behind the flag in 30db86a.
There was a problem hiding this comment.
When originally implementing this feature, i wasn't sure whether it was necessary to execute the original Next.js handler - it's not needed for the Netlify revalidate to work - but I left it in because it allows Next.js to validate the input and potentially throw an error, which will block the revalidate from happening. However, it was just a precaution (I can't think of a scenario where this would be necessary because our own revalidate API can also handle these errors) so i think it's fine to go with your rework. In fact, I don't think you need to put it behind the feature flag because the bulk of the change is already behind the this.netlifyConfig.revalidateToken condition, which itself is feature-flagged. You also don't need to await the original handler with that method, you can just return it (which is the how the default Next.js handler works).
There was a problem hiding this comment.
that's good context, thanks! If the on-demand revalidation stuff is still flagged, then we should definitely remove the second flag here to make things less complex. I opened a follow-up: #2113
| @@ -48,6 +47,8 @@ const makeApiHandler = ({ conf, app, pageRoot, page, NextServer }: MakeApiHandle | |||
| require.resolve('./pages.js') | |||
| } catch {} | |||
|
|
|||
| const NetlifyNextServer: NetlifyNextServerType = getNetlifyNextServer(NextServer) | |||
There was a problem hiding this comment.
commit messages mention trial and error process when this was added here - is this actually needed in api handler or just leftover from trials?
If it's needed - (not for this PR) I do wonder wether we should try to merge getApiHandler and getHandler into one as they become very similar now (not exactly the same) in future to make it easier to maintain going forward
There was a problem hiding this comment.
this is needed - the underlying problem was that if the plain NextServer impl is used, then revalidate fails because we're overriding the revalidate impl in NetlifyNextServer. In the past, getApiHandler was only used for scheduled functions + background functions, which won't use revalidate, so this wasn't a problem.
Agree that we should try to merge the two going forward. I've opened #2102 to track that work.
|
Can you let us know when it is safe for us to ask customers to test this functionality? We think that it still has pieces that aren't here yet based on the above listing of PR's and issues that mention this, but maybe I am misreading?
|
|
@Skn0tt please let us know when we can tell enterprise customers they can test this for us - I had assumed it was already rolled out to all, since an enterprise customer was auto-upgraded to 4.38.1 on wednesday (cf https://github.com/netlify/pillar-support/issues/568), but now seeing that this was not @orinokai 's understanding, so am looking for the authoritative answer :) |
|
If you really want to test this out, then you can set the Having said that, this will only impact API Routes, and most likely it won't help with large individual files, like the 2GB |
|
OK. I am hearing the implication that this work might not change much today for many users. But, presumably folks for whom it breaks something, they will let us know, so it is a good test for folks to try out if they have been struggling with larger lambdas just to see if it helps and ensure it doesn't hurt! |
Summary
Brings back split API Routes, but with faster bundling.
Splitting up API Routes into own Lambdas can result in reduced bundle size, which we were hoping for when it was first implemented. We had to revert that in
#1731, since it increased the bundling times by a lot.
ZISI added a
nonebundling strategy in netlify/zip-it-and-ship-it#1173. This allows us to skip tracing, which is very expensive. Luckily, Next.js already emits the.nft.jsonfiles that should contain all the tracing info we need!This PR will utilise these two, to bring us split-up API routes with fast bundling.
Test plan
I haven't done a lot of work in this repo before, but i'm hoping that our e2e tests cover all my changes! Ideally, there won't be any noticeable changes in behaviour.
I'm planning to do a staged rollout of this bundling strategy. Once we're happy with how it's working, we can see if we can apply the same to SSR routes.
Relevant links (GitHub issues, Notion docs, etc.) or a picture of cute animal
https://github.com/netlify/pod-compute/issues/252
Standard checks:
🧪 Once merged, make sure to update the version if needed and that it was published correctly.