fix: only split extended routes to decrease build times #1731
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ 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 netlify-plugin-nextjs-export-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. |
github-actions
bot
added
the
type: chore
nickytonline
changed the title
nickytonline
commented
Nov 1, 2022
✅ 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-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. |
nickytonline
force-pushed
the
nickytonline/unsplit-some-routes
branch
from
a582d74 to
3ba3160
Compare
✅ 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-edge-middleware 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 next-plugin-edge-middleware 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 nextjs-plugin-custom-routes-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
nickytonline
force-pushed
the
nickytonline/unsplit-some-routes
branch
from
1050817 to
948363c
Compare
nickytonline
commented
Nov 1, 2022
| "from": "/api/hello", | ||
| "status": 200, | ||
| "to": "/.netlify/functions/_api_hello-handler", | ||
| }, | ||
| Object { |
There was a problem hiding this comment.
Only generate custom rewrites for advanced API routes (background and scheduled functions).
There was a problem hiding this comment.
Where are we mapping the other API routes to the SSR handler?
There was a problem hiding this comment.
From what I understood of the previous implementation, the /api/* has a handler that catches all other requests.
{
from: `${basePath}/api/*`,
to: HANDLER_FUNCTION_PATH,
status: 200,
},
nickytonline
commented
Nov 1, 2022
There was a problem hiding this comment.
Looks like only the required functions are building now. I was seeing the old page routes generate functions still, but it looks like doing a git clean -dfx, npm install, then a fresh ntl build works.
────────────────────────────────────────────────────────────────
4. Functions bundling
────────────────────────────────────────────────────────────────
The Netlify Functions setting targets a non-existing directory: netlify/functions
Packaging Functions from .netlify/functions-internal directory:
- ___netlify-handler/___netlify-handler.js
- ___netlify-odb-handler/___netlify-odb-handler.js
- _api_hello-background-background/_api_hello-background-background.js
- _api_hello-scheduled-handler/_api_hello-scheduled-handler.js
- _ipx/_ipx.js
ascorbic
reviewed
Nov 2, 2022
| "from": "/api/hello", | ||
| "status": 200, | ||
| "to": "/.netlify/functions/_api_hello-handler", | ||
| }, | ||
| Object { |
There was a problem hiding this comment.
Where are we mapping the other API routes to the SSR handler?
Merged
Skn0tt
added a commit
that referenced
this pull request
Apr 20, 2023
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!
2 tasks
Skn0tt
added a commit
that referenced
this pull request
May 12, 2023
…2058) * feat: split up API Routes 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! * feat: load includedFiles for every page * refactor: extract function config logic * refactor: extract flag into own definition * feat: use "none" bundler for split-up api routes * feat: list some more dependencies * feat: use NFT to trace common required files * refactor: clean up a wee bit * fix: please don't include /sh * feat: enable flag by default, so tests use it * feat: add a naïve packing algo * feat: write rough sketch for packing lambdas * refactor: add constructor for APILambda * feat: pack handlers together into bundles * fix: linter * feat: exclude some heavy unneeded files * fix: trigger CI again, now that it supports `none` bundler * feat: remove code for old mechanism If we'll be doing a staged rollout, and our test suite covers most of the cases, we should be able to roll this out without a self-serve opt-out mechanism. * fix: remove test for deleted code * fix: ensure that react doesn't try to load development build * fix: move test directory into repo, so node_modules can be read * fix: snapshot with API redirects * fix: remove .only * fix: don't assert on _api_* * fix: another test * fix: apply NODE_ENV=prod to right generated handler * feat: remove nft tracing * feat: put change behind flag again * feat: source flag from plugin input * fix: add default value for featureflags * fix: default flag to true for testing * fix: revert changes to lockfiles * fix: eslint it/test * fix: lint * fix: revert distracting change * fix: now that we don't use nft anymore, we don't have to copy over node_modules * fix: swallow require.resolve errors for unit tests * fix: remove timing logs * fix: lint name * fix: lint * fix: isr needs _document.js * fix: add _app for ISR * fix: correct wrong output of some npm versions * fix: integration test For some reason, ZISI doesn't like relative paths in this integration test. We can fix it by using absolute paths. Since this PR moves API routes out of __netlify_handler, we can no longer make the assertion on x-nf-render-mode. * fix: assemble npm package path correctly, also for monorepos * fix: try what happens if we use next-netlify server * fix: check what happens when we skip revalidation * fix: dont let revalidate request time out * fix: send x-nextjs-cache header to prevent error * fix: update error message in test * fix: resolve relative paths based on data in required-server-files * fix: test * fix: keep manually-added `included_files` * fix: try something * fix: don't include unneeded _app pages in ISR * fix: finalize includedFiles before writing it onto netlifyConfig * chore: update comment * fix: exclude sass file in monorepos * Update packages/runtime/src/helpers/functions.ts * chore: remove comment * fix: update flag impl * refactor: use getRequiredServerFiles * chore: add comment on route[0] * fix: set NEXT_SPLIT_API_ROUTES in netlify.toml * fix: put updated revalidate behaviour behind flag * fix: supply splitApiRoutes in getHandler * fix: better run your code before committing it and embarrassing yourself --------- Co-authored-by: Nick Taylor <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Now we only create custom rewrites and functions for background and scheduled functions. Prior to this PR, we were splitting all API routes and creating separate functions and rewrites for them. This should decrease build times drastically (back to normal) for folks not using extended API routes (scheduled and background functions), and for those who are, the build time should be what it was plus whatever amount of time it takes to build the functions for the extended API routes.
Test plan
From a user facing perspective nothing has changed, so all dmoe sites and their corresponding tests should pass.
As well, the logs for the default demo site deploy preview, https://deploy-preview-1731--netlify-plugin-nextjs-demo.netlify.app/, should only create the following functions (see https://app.netlify.com/sites/netlify-plugin-nextjs-demo/deploys/63619a9da96eb900081e6cdf#L824-L831)
Relevant links (GitHub issues, Notion docs, etc.) or a picture of cute animal
Closes https://github.com/netlify/pod-ecosystem-frameworks/issues/284
Closes https://github.com/netlify/pod-ecosystem-frameworks/issues/282 potentially
Relates to #1495
Just a note that on my own site, these behave as expected
and these tests are still passing with the changes made.
https://github.com/netlify/next-runtime/blob/91ea2a67170bebee07db437d5496e58f2911e81e/cypress/integration/default/api.spec.ts#L9-L18
Standard checks:
🧪 Once merged, make sure to update the version if needed and that it was published correctly.