Closes#48479.
Couldn't find the source for the Next.js beta docs, so I didn't add documentation.
Co-authored-by: Jiachi Liu <4800338+huozhi@users.noreply.github.com>
This is a follow-up to log both conflicting paths & a link to route group docs, which I believe is the only scenario someone could trigger this
- https://github.com/vercel/next.js/pull/53752
Currently using server actions on a page or using edge runtime causes
that page to bail out of ISR or static generation so this adds warnings
to make users aware of this.
x-ref: [slack
thread](https://vercel.slack.com/archives/C03KAR5DCKC/p1690816539472449)
---------
Co-authored-by: Zack Tanner <zacktanner@gmail.com>
### What?
Fixing outdated import mapping with the latest `lucide-react` changes. See [lucide-icons/lucide@v0.264.0](https://github.com/lucide-icons/lucide/releases/tag/v0.264.0)
Closes: lucide-icons/lucide#1482
### Why?
Import mapping was outdated
### How?
By updating the default config and the tests.
## Notes
Maintainer here from @lucide-icons.
To this day I didn't know the Nextjs default config file has predefined hardcoded modularize imports.
I was a bit surprised that there is a default mapping for libraries that could change in the future like Lucide.
I'm not in favor of this hardcoded "modularize imports", because this creates a hard dependency on libraries. With Lucide, we still have not reached 1.0 and will sometimes rename icons and add aliases for them. So issues will occur in the future, and constantly updating this config doesn't feel right.
Is there, not a better way? Like for example, a package exposing a config file containing the import mapping?
This way import map will automatically be in sync with changes in a package.
Let me know what you think of this.
Currently all scripts that are required for every page are loaded as
part of the bootstrap scripts API in React. Unfortunately this loads
them all as sync scripts and thus requires preloading which increases
their priority higher than they might otherwise be causing things like
images to load later than desired, blocking paint. We can improve this
by only using one script for bootstrapping and having the rest
pre-initialized. This only works because all of these scripts are
webpack runtime or chunks and can be loaded in any order asynchronously.
With this change we should see improvements in LCP and other metrics as
preloads for images are favored over loading scripts
Co-authored-by: Steven <steven@ceriously.com>
This fixes some tests that were disabled due to a missing `page` segment for the corresponding slots in `/parallel/nested`.
While fixing, I also noticed if you accidentally create two pages that resolve to the same URL segment (which is fairly easy to do accidentally do with route groups), we were throwing an unhelpful error of "Cannot find module: '<snip>/page_client-reference-manifest.js'" when building (and fail silently in dev). For example, this scenario was throwing a manifest error:
```
app
(groupa)
page.tsx
(groupb)
page.tsx
```
This will now throw with a more helpful error when resolving parallel segments if the page segment was already resolved. This also re-enables the disabled tests.
Closes NEXT-1440
Fixes#53569 (by virtue of throwing a more helpful error)
In the current version of Next.js there are 4 processes when running in
production:
- Server
- Routing
- Rendering Pages Router
- Rendering App Router
This setup was introduced in order to allow App Router and Pages Router
to use different versions of React (i.e. Server Actions currently
requires react@experimental to function). I wrote down more on why these
processes exist in this comment:
https://github.com/vercel/next.js/issues/49929#issuecomment-1637185156
This PR combines the Server and Routing process into one handler, as the
"Server" process was only proxying to the Routing process. In my testing
this caused about ~2x the amount of memory per request as the response
body was duplicated between the processes. This was especially visible
in the case of that memory leak in Node.js 18.16 as it grew memory usage
on both sides quickly.
In the process of going through these changes I found a couple of other
bugs like the propagation of values to the worker processes not being
awaited
([link](https://github.com/vercel/next.js/pull/53523/files#diff-0ef09f360141930bb03263b378d37d71ad9432ac851679aeabc577923536df84R54))
and the dot syntax for propagation was not functioning.
It also seemed there were a few cases where watchpack was used that
would cause many more files to be watched than expected, for now I've
removed those cases, specifically the "delete dir while running" and
instrument.js hmr (instrument.js is experimental). Those tests have been
skipped for now until we can revisit them to verfiy it
I've also cleaned up the types a bit while I was looking into these
changes.
### Improvement
⚠️ Important preface to this, measuring memory usage / peak usage is not
super reliable especially when JS gets garbage collected. These numbers
are just to show the rough percentage of less memory usage.
#### Baseline
Old:
```
next-server: 44.8MB
next-router-worker: 57.5MB
next-render-worker-app: 39,6MB
next-render-worker-pages: 39,1MB
```
New:
```
next-server: Removed
next-router-worker: 64.4MB
next-render-worker-app: 43.1MB (Note: no changes here, this shows what I meant by rough numbers)
next-render-worker-pages: 42.4MB (Note: no changes here, this shows what I meant by rough numbers)
```
Overall win: ~40MB (process is removed)
#### Peak usage
Old:
```
next-server: 118.6MB
next-router-worker: 223.7MB
next-render-worker-app: 32.8MB (I ran the test on a pages application in this case)
next-render-worker-pages: 101.1MB
```
New:
```
next-server: Removed
next-router-worker: 179.1MB
next-render-worker-app: 33.4MB
next-render-worker-pages: 117.5MB
```
Overall win: ~100MB (but it scales with requests so it was about ~50% of
next-router-worker constantly)
Related: #53523
---------
Co-authored-by: JJ Kasper <jj@jjsweb.site>
When using imports from `next/headers` in a layout or page,
`StaticGenerationBailout` will throw an error to indicate Next.js should
fallback to dynamic rendering. However, when async context is lost, this
error is uncaught and leads to a confusing error message at build time.
This attempts to improve DX surrounding this error by linking out to a
page that explains when it might happen. I've also tweaked
`StaticGenerationBailout` to always throw a fully descriptive reason as
opposed to just `DynamicServerError: Dynamic server usage: cookies`
Closes NEXT-1181
Fixes#49373
---------
Co-authored-by: Lee Robinson <me@leerob.io>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
### What?
`ReactDOM.preload` is available in `react-dom@experimental` builds. If it's not available, we should fall back to `Head`+`link`
### Why?
Since `ReactDOM.preload` is only available in `react-dom@experimental` builds, certain environments (like Jest or [Storybook](https://github.com/storybookjs/storybook/issues/23661)) might have a version of `react-dom` installed that won't work with `preload()`
### How?
Closes NEXT-1482
Fixes#53272
See also: https://github.com/storybookjs/storybook/issues/23661
Co-authored-by: Steven <229881+styfle@users.noreply.github.com>
Edge runtime doesn't have `/404` entry points as not-found, if server hits 404 it's hitting `/_not-found` entry point of edge bundles unlike nodejs server rendering.
In app-render it needs the information that when `pathname` (or can call `pagePath`) is `/404` it can render the `not-found.js` components properly
Separate the not-found test suite into 2 parts, a basic case testing all urls with `not-found.js` boundary, and another conflict route case with `not-found.js` with `not-found/` route at the same time
Fixes#53652Fixes#53210
Adding GoogleMapsEmbed and YoutubeEmber components into `@next/third-parties`
Adding tests and README
Each component is tagged with `data-ntpc` attribute
cc: @kara @housseindjirdeh @huozhi @gnoff
Co-authored-by: Jiachi Liu <4800338+huozhi@users.noreply.github.com>
## Vendoring
Updates all module resolvers (node, webpack, nft for entrypoints, and nft for next-server) to consider whether vendored packages are suitable for a given resolve request and resolves them in an import semantics preserving way.
### Problem
Prior to the proposed change, vendoring has been accomplished but aliasing module requests from one specifier to a different specifier. For instance if we are using the built-in react packages for a build/runtime we might replace `require('react')` with `require('next/dist/compiled/react')`.
However this aliasing introduces a subtle bug. The React package has an export map that considers the condition `react-server` and when you require/import `'react'` the conditions should be considered and the underlying implementation of react may differ from one environment to another. In particular if you are resolving with the `react-server` condition you will be resolving the `react.shared-subset.js` implementation of React. This aliasing however breaks these semantics because it turns a bare specifier resolution of `react` with path `'.'` into a resolution with bare specifier `next` with path `'/dist/compiled/react'`. Module resolvers consider export maps of the package being imported from but in the case of `next` there is no consideration for the condition `react-server` and this resolution ends up pulling in the `index.js` implementation inside the React package by doing a simple path resolution to that package folder.
To work around this bug there is a prevalence of encoding the "right" resolution into the import itself. We for instance directly alias `react` to `next/dist/compiled/react/react.shared-subset.js` in certain cases. Other times we directly specify the runtime variant for instance `react-server-dom-webpack/server.edge` rather than `react-server-dom-wegbpack/server`, bypassing the export map altogether by selecting the runtime specific variant. However some code is meant to run in more than one runtime, for instance anything that is part of the client bundle which executes on the server during SSR and in the browser. There are workaround like using `require` conditionally or `import(...)` dynamically but these all have consequences for bundling and treeshaking and they still require careful consideration of the environment you are running in and which variant needs to load.
The result is that there is a large amount of manual pinning of aliases and additional complexity in the code and an inability to trust the package to specify the right resolution potentially causing conflicts in future versions as packages are updated.
It should be noted that aliasing is not in and of itself problematic when we are trying to implement a sort of lightweight forking based on build or runtime conditions. We have good examples of this for instance with the `next/head` package which within App Router should export a noop function. The problem is when we are trying to vendor an entire package and have the package behave semantically the same as if you had installed it yourself via node_modules
### Solution
The fix is seemingly straight forward. We need to stop aliasing these module specifiers and instead customize the resolution process to resolve from a location that will contain the desired vendored packages. We can then start simplifying our imports to use top level package resources and generally and let import conditions control the process of providing the right variant in the right context.
It should be said that vendoring is conditional. Currently we only vendor react pacakges for App Router runtimes. The implementation needs to be able to conditionally determine where a package resolves based on whether we're in an App Router context vs a Page Router one.
Additionally the implementation needs to support alternate packages such as supporting the experimental channel for React when using features that require this version.
### Implementation
The first step is to put the vendored packages inside a node_modules folder. This is essential to the correct resolving of packages by most tools that implement module resolution. For packages that are meant to be vendored, meaning whole package substitution we move the from `next/(src|dist)/compiled/...` to `next/(src|dist)/vendored/node_modules`. The purpose of this move is to clarify that vendored packages operate with a different implementation. This initial PR moves the react dependencies for App Router and `client-only` and `server-only` packages into this folder. In the future we can decide which other precompiled dependencies are best implemented as vendored packages and move them over.
It should be noted that because of our use of `JestWorker` we can get warnings for duplicate package names so we modify the vendored pacakges for react adding either `-vendored` or `-experimental-vendored` depending on which release channel the package came from. While this will require us to alter the request string for a module specifier it will still be treating the react package as the bare specifier and thus use the export map as required.
#### module resolvers
The next thing we need to do is have all systems that do module resolution implement an custom module resolution step. There are five different resolvers that need to be considered
##### node runtime
Updated the require-hook to resolve from the vendored directory without rewriting the request string to alter which package is identified in the bare specifier. For react packages we only do this vendoring if the `process.env.__NEXT_PRIVATE_PREBUNDLED_REACT` envvar is set indicating the runtime is server App Router builds. If we need a single node runtime to be able to conditionally resolve to both vendored and non vendored versions we will need to combine this with aliasing and encode whether the request is for the vendored version in the request string. Our current architecture does not require this though so we will just rely on the envvar for now
##### webpack runtime
Removed all aliases configured for react packages. Rely on the node-runtime to properly alias external react dependencies. Add a resolver plugin `NextAppResolverPlugin` to preempt perform resolution from the context of the vendored directory when encountering a vendored eligible package.
##### turbopack runtime
updated the aliasing rules for react packages to resolve from the vendored directory when in an App Router context. This implementation is all essentially config b/c the capability of doing the resolve from any position (i.e. the vendored directory) already exists
##### nft entrypoints runtime
track chunks to trace for App Router separate from Pages Router. For the trace for App Router chunks use a custom resolve hook in nft which performs the resolution from the vendored directory when appropriate.
##### nft next-server runtime
The current implementation for next-server traces both node_modules and vendored version of packages so all versions are included. This is necessary because the next server can run in either context (App vs Page router) and may depend on any possible variants. We could in theory make two traces rather than a combined one but this will require additional downstream changes so for now it is the most conservative thing to do and is correct
Once we have the correct resolution semantics for all resolvers we can start to remove instances targeting our precompiled instances for instance making `import ... from "next/dist/compiled/react-server-dom-webpack/client"` and replacing with `import ... from "react-server-dom-webpack/client"`
We can also stop requiring runtime specific variants like `import ... from "react-server-dom-webpack/client.edge"` replacing it with the generic export `"react-server-dom-webpack/client"`
There are still two special case aliases related to react
1. In profiling mode (browser only) we rewrite `react-dom` to `react-dom/profiling` and `scheduler/tracing` to `scheduler/tracing-profiling`. This can be moved to using export maps and conditions once react publishses updates that implement this on the package side.
2. When resolving `react-dom` on the server we rewrite this to `react-dom/server-rendering-stub`. This is to avoid loading the entire react-dom client bundle on the server when most of it goes unused. In the next major react will update this top level export to only contain the parts that are usable in any runtime and this alias can be dropped entirely
There are two non-react packages currently be vendored that I have maintained but think we ought to discuss the validity of vendoring. The `client-only` and `server-only` packages are vendored so you can use them without having to remember to install them into your project. This is convenient but does perhaps become surprising if you don't realize what is happening. We should consider not doing this but we can make that decision in another discussion/PR.
#### Webpack Layers
One of the things our webpack config implements for App Router is layers which allow us to have separate instances of packages for the server components graph and the client (ssr) graph. The way we were managing layer selection was a but arbitrary so in addition to the other webpack changes the way you cause a file to always end up in a specific layer is to end it with `.serverlayer`, `.clientlayer` or `.sharedlayer`. These act as layer portals so something in the server layer can import `foo.clientlayer` and that module will in fact be bundled in the client layer.
#### Packaging Changes
Most package managers are fine with this resolution redirect however yarn berry (yarn 2+ with PnP) will not resolve packages that are not defined in a package.json as a dependency. This was not a problem with the prior strategy because it was never resolving these vendored packages it was always resolving the next package and then just pointing to a file within it that happened to be from react or a related package.
To get around this issue vendored packages are both committed in src and packed as a tgz file. Then in the next package.json we define these vendored packages as `optionalDependency` pointing to these tarballs. For yarn PnP these packed versions will get used and resolved rather than the locally commited src files. For other package managers the optional dependencies may or may not get installed but the resolution will still resolve to the checked in src files. This isn't a particularly satisfying implemenation and if pnpm were to be updated to have consistent behavior installing from tarballs we could actually move the vendoring entirely to dependencies and simplify our resolvers a fair bit. But this will require an upstream chagne in pnpm and would take time to propogate in the community since many use older versions
#### Upstream Changes
As part of this work I landed some other changes upstream that were necessary. One was to make our packing use `npm` to match our publishing step. This also allows us to pack `node_modules` folders which is normally not supported but is possible if you define the folder in the package.json's files property.
See: #52563
Additionally nft did not provide a way to use the internal resolver if you were going to use the resolve hook so that is now exposed
See: https://github.com/vercel/nft/pull/354
#### additional PR trivia
* When we prepare to make an isolated next install for integration tests we exclude node_modules by default so we have a special case to allow `/vendored/node_modules`
* The webpack module rules were refactored to be a little easier to reason about and while they do work as is it would be better for some of them to be wrapped in a `oneOf` rule however there is a bug in our css loader implementation that causes these oneOf rules to get deleted. We should fix this up in a followup to make the rules a little more robuts.
## Edits
* I removed `.sharedlayer` since this concept is leaky (not really related to the client/server boundary split) and it is getting refactored anyway soon into a precompiled runtime.
This removes the `basePath` from the output of `usePathname`. Previously this always resulted in hydration errors, this now strips the `basePath` when it's found/configured.
Now when you configure `basePath`, you don't have to factor it into your application logic and can instead rely on the values always returning the pathname without it.
Fixes#46562
### What & Why
The dynamic not-found boundary didn't work as expected as it was using the `pathname` to match how many levels of the segements should be matched. For dynamic routes this doesn't work, unlike normal page, the unmatched segment can also hit the not found boundary in the same level.
### How
Use `segment` of loader tree nodes to determine if not-found boundary searching is reached to the end instead of using `pathname`.
> NOTE: For production `/_not-found` case since it's a valid page on production and still has the original tree, so we handle that as a special case to use the not found loader tree (with empty child routes) to render.
Fixes#53344
### What & Why
Using `notFound()` in `generateMetadata()` or in page will lead to unacught global not found error when you do navigation, this is because `head` cache is actually not inside the error boundary as designed to stay in the beginning of the content. But in this way we won't be able to catch the notFound error thrown from metadata API. So we created a new approach that can separate the error itself from metadata result, and throw as error under the error boundaries, and metadata itself will always be safe to render.
### How
We use a new promise that resolve the `error` thrown from metadata resolving, and let the render result always return successfully and always has a value, using default metadata when there's error thrown. Create two components, one rendering the metadata tags, and another `MetadataOutlet` to throw the error when the resolving is failed but rendered under error boundaries.
```jsx
const [MetadataTree, MetadataOutlet] = createMetadataComponents(/*...*/)
const ComponentTree = createComponentTree({ metadataOutlet: <MetadataOutlet /> })
renderRSC(
<AppRourer head={<MetadataTree />}>
<ComponentTree />
</AppRourer>
)
```
`metadataOutlet` will stay next to page component in the layout hierarchy
discuessed with @gnoff
Minor changes:
* When there's rendering root layout layer, not found boundary should be `DefaultNotFound` if the custom one doesn't exisit
Fixes#53371Fixes#53149
This adds support for `--use-bun` to `create-next-app` to use `bun
install` when bootstrapping a new project.
```
npx create-next-app --use-bun
```
As with Yarn and pnpm, it reads from `npm_config_user_agent` to
determine if the user ran `bunx create-next-app`. If so, it defaults to
using Bun.
```sh
bunx create-next-app
```
## For Contributors
### Improving Documentation
- [x] Run `pnpm prettier-fix`
- [x] `pnpm build && pnpm lint`
- [x] Added test to
`test/integration/create-next-app/package-manager.test.ts`
---------
This attempts to fix an issue where fetch requests were not being
deduped on the first request to the page (but subsequent requests were
properly deduped).
This seems to be caused by the async context from
`staticGenerationStore` being lost by the time the patched fetch is
called, and so it was falling back to a regular fetch and skipping the
cache. This attempts to fix that by falling back to
`fetch.__nextGetStaticStore()`.
[slack
x-ref](https://vercel.slack.com/archives/C03KAR5DCKC/p1691007445597619)
Closes#49607. Since `ReactDOM.preload`s might be called multiple times during the rendering process, we need to ensure the timestamp queries are stable across the request so Flight can properly deduplicate them.
### What?
This PR makes it so calling `experimental_useOptimistic` throws an error telling you it only works in a client component. Because the Next docs have an example of renaming it into `useOptimistic` in the import, I also added that as a forbidden import. There may be a better way to do this, if so, please let me know.
Fixes#53312
### Why?
Currently, the error you get says `(0 , react__WEBPACK_IMPORTED_MODULE_1__.experimental_useOptimistic) is not a function or its return value is not iterable`. This is misleading.
<img width="1043" alt="Screenshot 2023-07-28 at 3 30 10 PM" src="https://github.com/vercel/next.js/assets/12662580/ee16fd84-633d-47a1-8db4-cfc050546614">
### How?
Adds `experimental_useOptimistic` to the lists of forbidden imports. Adds some specific tests around this, but I'm not sure they're necessary, looking at how the other tests are written.
Co-authored-by: Zack Tanner <1939140+ztanner@users.noreply.github.com>
## What
Using methods from `next/headers` in middleware would throw a `requestAsyncStorage` invariant. Additionally, using `draftMode()` in middleware/an edge runtime is not possible
## Why
We do not expose `requestAsyncStorage` to the middleware adapter. Also, the prerender manifest wasn't available to the `EdgeRouteModuleWrapper` & middleware adapter, so it wasn't possible to enable/disable it.
## How
This makes `requestAsyncStorage` available for middleware, and makes the preview mode data available from build to the edge runtime/middleware.
Fixes#52557
### What?
* fixes problems in Next.rs API introduced by #52846
* adds test infrastructure for experimental turbo testing
* adds two test cases to verify the infrastructure
* add grouping of output logs in run-tests
* simplify template loading
### Why?
### How?
Hi!
In previous versions (13.4.2 and earlier) in the custom server I could
do the following:
```
const parsedUrl = parse(req.url, true);
const { pathname } = parsedUrl;
if (pathname === '/c') {
await handle(req, res, parse('/b', true));
} else {
await handle(req, res, parsedUrl);
}
```
Of course, you can replace `handle` with `app.render` in the attached
example, but in practice it is convenient to put the definition of
`parsedUrl` into middleware and substitute the necessary `parsedUrl` in
exceptional situations.
This was broken in #49805.
I'm not sure if this use of request handler is expected, but it has
always worked and people have used it (probably due to lack of
documentation about the difference between `app.render` and
`app.getRequestHandler()`). So the change looks breaking, and I don't
think it should appear in minor versions.
---------
Co-authored-by: Tim Neutkens <tim@timneutkens.nl>
## What?
I was investigating reports of server actions with `revalidatePath` /
`revalidateTag` not invalidating the client-side router cache. Managed
to reproduce the issue here:
https://github.com/timneutkens/server-actions-test (requires Vercel KV
to run).
While looking at the reducer I noticed a few things that seemed off:
- setTimeout to trigger another `dispatch` on the same reducer with the
fetched data. This means that it would not be part of the same React
Transition.
- redirects weren't applying the data that comes back from the server
(that allows for single hop navigation)
- prefetchCache was invalidated, but the router cache which is used for
back/forward navigation was not invalidated, causing back/forward to not
get the new data.
- all changes in the action-reducer mutate. The part that shouldn't
mutate was part of that setTimeout dispatch.
This PR aims to solve all of these by reworking the actions reducer to
be handled similarly to `router.refresh()`. Since `router.refresh()` was
already modeled to be similar to `revalidatePath('/')`.
The flow is now more like the other reducers:
- Fetch data
- Wait for the data to come back
- Apply the data to the cache and keep it in a mutable variable
- Return the new cache and otherwise values like the url
In particular the actions reducer handles a few extra specifics:
- Resolving the server action promise, so that the value is passed to
the application code waiting for the result.
- Applying redirects from `redirect()` calls.
- Invalidating the router cache
## Followup changes
- Currently when calling `revalidatePath('/dashboard')` the entire
router cache is invalidated instead of only `/dashboard` and further
down, this is not in scope for this PR but still has to be added.
- `notFound()`, I'm not sure how this is supposed to work exactly, it
doesn't really make sense to me to allow it in server actions.
Kudos @feedthejim for helping investigate for this PR 😌
<!-- Thanks for opening a PR! Your contribution is much appreciated.
To make sure your PR is handled as smoothly as possible we request that
you follow the checklist sections below.
Choose the right checklist for the change(s) that you're making:
## For Contributors
### Improving Documentation
- Run `pnpm prettier-fix` to fix formatting issues before opening the
PR.
- Read the Docs Contribution Guide to ensure your contribution follows
the docs guidelines:
https://nextjs.org/docs/community/contribution-guide
### Adding or Updating Examples
- The "examples guidelines" are followed from our contributing doc
https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md
- Make sure the linting passes by running `pnpm build && pnpm lint`. See
https://github.com/vercel/next.js/blob/canary/contributing/repository/linting.md
### Fixing a bug
- Related issues linked using `fixes #number`
- Tests added. See:
https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs
- Errors have a helpful link attached, see
https://github.com/vercel/next.js/blob/canary/contributing.md
### Adding a feature
- Implements an existing feature request or RFC. Make sure the feature
request has been accepted for implementation before opening a PR. (A
discussion must be opened, see
https://github.com/vercel/next.js/discussions/new?category=ideas)
- Related issues/discussions are linked using `fixes #number`
- e2e tests added
(https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs
- Documentation added
- Telemetry added. In case of a feature if it's used or not.
- Errors have a helpful link attached, see
https://github.com/vercel/next.js/blob/canary/contributing.md
## For Maintainers
- Minimal description (aim for explaining to someone not on the team to
understand the PR)
- When linking to a Slack thread, you might want to share details of the
conclusion
- Link both the Linear (Fixes NEXT-xxx) and the GitHub issues
- Add review comments if necessary to explain to the reviewer the logic
behind a change
### What?
### Why?
### How?
Closes NEXT-
Fixes #
-->
### What?
Change the `favicon.ico` metadata `sizes` property from always `"any"`
to using the actual size possible
### Why?
When chrome/firefox browsers search for icon it needs the proper
metadata to determine which one to use, giving `favicon.ico` sizes with
`"any"` will prevent it loading the `icon.svg` as default icon when
available.
Changing to set proper size of `favicon.ico` (as the `favicon.ico` sizes
could be 16x16, 32x32, etc.) fixes the issue.
It works (loading svg icon) for chrome and firefox:
Firefox
<img width="505" alt="image"
src="https://github.com/vercel/next.js/assets/4800338/6873e595-479d-4d9e-ae5c-39e5f938fcf5">
Chrome
<img width="460" alt="image"
src="https://github.com/vercel/next.js/assets/4800338/03bbe4c7-ef76-4f34-a611-337b0d4b97a3">
It loads favicon.ico for Safari:
Using the `test/e2e/app-dir/metadata` app it shows the favicon.ico for
Safari
<img width="1000" alt="image"
src="https://github.com/vercel/next.js/assets/4800338/92cc8714-ea5e-432d-8144-2a4a42ee4ce2">
Can't have it as an e2e test as I tested it always loads the
`favicon.ico` in headless mode which can't determine the behaviors
Fixes#52002
Co-authored-by: JJ Kasper <jj@jjsweb.site>
This PR reverts a change that removed the `content-length` header filtering from the req made by the actions when redirecting. This change made some tests flaky and presumably also broke server actions in subtle ways.
There's still one other bug when redirecting after revalidating that will happen in you revalidate a page that was already rendered before where we will still show stale content. @timneutkens is fixing that one.
NEXT-1483
