-
Notifications
You must be signed in to change notification settings - Fork 89
fix: Return MiddlewareResponse obj for rewrite #2159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
✅ 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-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. |
✅ 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-i18next-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 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. |
All the tests should be passing on this, there might have been an update recently with the app-dir tests. Will have to review once I'm back. |
@@ -2,7 +2,7 @@ | |||
import spawn from 'cross-spawn' | |||
import { existsSync, readFileSync, unlinkSync, writeFileSync } from 'fs' | |||
import { writeFile } from 'fs-extra' | |||
import { fetch as undiciFetch } from 'undici' | |||
import { fetch as undiciFetch } from 'next/dist/compiled/undici' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we never figured out why the undici
dep was no longer included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It happened here c295bc2
it got removed from package-lock but it looks like it was not set as a dep in the package.json.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we move to Node.js 18, fetch
is native, so this import won't be required.
Co-authored-by: Nick Taylor <nick@iamdeveloper.com>
update pageProps testing Co-authored-by: Nick Taylor <nick@iamdeveloper.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Description
Fixes https://github.com/netlify/pod-ecosystem-frameworks/issues/510
We are now returning the MiddlewareResponse from the request.rewrite() instead of NextResponse. This will allow us to utilize the setPageProp and replaceText method.
This PR is to get the initial issue resolved, we will continue the work in another PR for the init parameter.
Documentation
Tests
This page should be rewritten with the cat ad and message with geo location
You can test this change yourself like so:
Relevant links (GitHub issues, etc.) or a picture of cute animal