Skip to content
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

fix revalidation/refresh behavior with parallel routes #63607

Merged
merged 3 commits into from
Mar 28, 2024

Conversation

ztanner
Copy link
Member

@ztanner ztanner commented Mar 22, 2024

(Original PR #63608 was erroneously merged into this branch. As a result, I've copied the notes from it below.)

What

When a parallel segment in the current router tree is no longer "active" during a soft navigation (ie, no longer matches a page component on the particular route), it remains on-screen until the page is refreshed, at which point it would switch to rendering the default.tsx component. However, when revalidating the router cache via router.refresh, or when a server action finishes & refreshes the router cache, this would trigger the "hard refresh" behavior. This would have the unintended consequence of a 404 being triggered (which is the default behavior of default.tsx) or inactive segments disappearing unexpectedly.

Why

When the router cache is refreshed, it currently fetches new data for the page by fetching from the current URL the user is on. This means that the server will never respond with the data it needs if the segment wasn't "activated" via the URL we're fetching from, as it came from someplace else. Instead, the server will give us data for the default.tsx component, which we don't want to render when doing a soft refresh.

How

This updates the FlightRouterState to encode information about the URL that caused the segment to become active. That way, when some sort of revalidation event takes place, we can both refresh the data for the current URL (existing handling), and recursively refetch segment data for anything that was still present in the tree but requires fetching from a different URL. We patch this new data into the tree before committing the final CacheNode to the router.

Note: I re-used the existing refresh and url arguments in FlightRouterState to avoid introducing more options to this data structure that is already a bit tricky to work with. Initially I was going to re-use "refetch" as-is, which seemed to work ok, but I'm worried about potential implications of this considering they have different semantics. In an abundance of caution, I added a new marker type ("refresh", alternative suggestions welcome).

This has some trade-offs: namely, if there are a lot of different segments that are in this stale state that require data from different URLs, the refresh is going to be blocked while we fetch all of these segments. Having to do a separate round-trip for each of these segments could be expensive. In an ideal world, we'd be able to enumerate the segments we'd want to refetch and where they came from, so it could be handled in a single round-trip. There are some ideas on how to improve per-segment fetching which are out of scope of this PR. However, due to the implicit contract that middleware.ts creates with URLs, we still need to identify these resources by URLs.

Other Notes:

applyRouterStatePatchToTree had been refactored to support the case of not skipping the __DEFAULT__ segment, so that router.refresh or revalidating in a server action wouldn't break the router. (More details in this #59585)

This was a stop-gap and not an ideal solution, as this behavior means router.refresh() would effectively behave like reloading the page, where "stale" segments (ones that went from __PAGE__ -> __DEFAULT__) would disappear.

Fixes #60815
Fixes #60950
Fixes #51711
Fixes #51714
Fixes #58715
Fixes #60948
Fixes #62213
Fixes #61341

Closes NEXT-1845
Closes NEXT-2030

Closes NEXT-2903

Copy link
Member Author

ztanner commented Mar 22, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @ztanner and the rest of your teammates on Graphite Graphite

@ijjk
Copy link
Member

ijjk commented Mar 22, 2024

Failing test suites

Commit: 42e6cc6

pnpm test test/integration/amp-export-validation/test/index.test.js

  • AMP Validation on Export > production mode > should have shown errors during build
Expand output

● AMP Validation on Export › production mode › should have shown errors during build

expect(received).toMatch(expected)

Expected pattern: /error.*The mandatory attribute 'height' is missing in tag 'amp-video'\./
Received string:  "  ▲ Next.js 14.2.0-canary.46·
   Linting and checking validity of types ...
   Creating an optimized production build ...
 ✓ Compiled successfully
   Collecting page data ...
   Generating static pages (0/8) ...
   Generating static pages (2/8)··
   Generating static pages (4/8)··
   Generating static pages (6/8)··
 ✓ Generating static pages (8/8)·
 ⚠ The Next.js plugin was not detected in your ESLint configuration. See https://nextjs.org/docs/basic-features/eslint#migrating-existing-config·
Error occurred prerendering page \"/cat\". Read more: https://nextjs.org/docs/messages/prerender-error·
Error: Could not read from https://cdn.ampproject.org/v0/validator_wasm.js - aborted

  15 |  * See the License for the specific language governing permissions and
  16 |  * limitations under the license.
> 17 |  */const t=__nccwpck_require__(915);const r=__nccwpck_require__(147);const i=__nccwpck_require__(685);const s=__nccwpck_require__(687);const o=__nccwpck_require__(17);const a=__nccwpck_require__(642);const l=__nccwpck_require__(263);const u=__nccwpck_require__(477);const c=__nccwpck_require__(310);const h=__nccwpck_require__(837);const p=__nccwpck_require__(144);const f="amphtml-validator";function hasPrefix(e,t){return e.indexOf(t)==0}function isHttpOrHttpsUrl(e){return hasPrefix(e,"http://")||hasPrefix(e,"https://")}function readFromFile(e){return new l((function(t,n){r.readFile(e,"utf8",(function(e,r){if(e){n(e)}else{t(r.trim())}}))}))}function readFromReadable(e,t){return new l((function(n,r){const i=[];t.setEncoding("utf8");t.on("data",(function(e){i.push(e)}));t.on("end",(function(){n(i.join(""))}));t.on("error",(function(t){r(new Error("Could not read from "+e+" - "+t.message))}))}))}function readFromStdin(){return readFromReadable("stdin",process.stdin).then((function(e){process.stdin.resume();return e}))}function readFromUrl(e,t){return new l((function(n,r){const o=hasPrefix(e,"http://")?i:s;const a=o.request(e,(function(t){if(t.statusCode!==200){t.resume();r(new Error("Unable to fetch "+e+" - HTTP Status "+t.statusCode))}else{n(t)}}));a.setHeader("User-Agent",t);a.on("error",(function(t){r(new Error("Unable to fetch "+e+" - "+t.message))}));a.end()})).then(readFromReadable.bind(null,e))}function ValidationResult(){this.status="UNKNOWN";this.errors=[]}function ValidationError(){this.severity="UNKNOWN_SEVERITY";this.line=1;this.col=0;this.message="";this.specUrl=null;this.code="UNKNOWN_CODE";this.params=[]}function Validator(e){this.sandbox=p.createContext();try{new p.Script(e).runInContext(this.sandbox)}catch(e){throw new Error("Could not instantiate validator_wasm.js - "+e.message)}}Validator.prototype.init=function(){if(this.sandbox.amp.validator.init){return this.sandbox.amp.validator.init()}else{return l.resolve(undefined)}};Validator.prototype.validateString=function(e,t){const n=this.sandbox.amp.validator.validateString(e,t);const r=new ValidationResult;r.status=n.status;for(let e=0;e<n.errors.length;e++){const t=n.errors[e];const i=new ValidationError;i.severity=t.severity;i.line=t.line;i.col=t.col;i.message=this.sandbox.amp.validator.renderErrorMessage(t);i.specUrl=t.specUrl;i.code=t.code;i.params=t.params;r.errors.push(i)}return r};const m={};function getInstance(e,t){const n=e||"https://cdn.ampproject.org/v0/validator_wasm.js";const r=t||f;if(m.hasOwnProperty(n)){return l.resolve(m[n])}const i=isHttpOrHttpsUrl(n)?readFromUrl(n,r):readFromFile(n);return i.then((function(e){let t;try{t=new Validator(e)}catch(e){throw e}m[n]=t;return t})).then((function(e){return e.init().then((()=>e))}))}e.getInstance=getInstance;function newInstance(e){return new Validator(e)}e.newInstance=newInstance;function logValidationResult(e,n,r){if(n.status==="PASS"){process.stdout.write(e+": "+(r?t.green("PASS"):"PASS")+"\n")}for(let i=0;i<n.errors.length;i++){const s=n.errors[i];let o=e+":"+s.line+":"+s.col+" ";if(r){o+=(s.severity==="ERROR"?t.red:t.magenta)(s.message)}else{o+=s.message}if(s.specUrl){o+=" (see "+s.specUrl+")"}process.stderr.write(o+"\n")}}function main(){a.usage("[options] <fileOrUrlOrMinus...>\n\n"+'  Validates the files or urls provided as arguments. If "-" is\n'+"  specified, reads from stdin instead.").option("--validator_js <fileOrUrl>","The Validator Javascript.\n"+"  Latest published version by default, or\n"+"  dist/validator_minified.js (built with build.py)\n"+"  for development.","https://cdn.ampproject.org/v0/validator_wasm.js").option("--user-agent <userAgent>","User agent string to use in requests.",f).option("--html_format <AMP|AMP4ADS|AMP4EMAIL>","The input format to be validated.\n"+"  AMP by default.","AMP").option("--format <color|text|json>","How to format the output.\n"+'  "color" displays errors/warnings/success in\n'+"          red/orange/green.\n"+'  "text"  avoids color (e.g., useful in terminals not\n'+"          supporting color).\n"+'  "json"  emits json corresponding to the ValidationResult\n'+"          message in validator.proto.","color").parse(process.argv);const e=a.opts();if(e.length===0){a.outputHelp();process.exit(1)}if(e.html_format!=="AMP"&&e.html_format!=="AMP4ADS"&&e.html_format!=="AMP4EMAIL"){process.stderr.write('--html_format must be set to "AMP", "AMP4ADS", or "AMP4EMAIL".\n',(function(){process.exit(1)}))}if(e.format!=="color"&&e.format!=="text"&&e.format!=="json"){process.stderr.write('--format must be set to "color", "text", or "json".\n',(function(){process.exit(1)}))}const n=[];for(let t=0;t<a.args.length;t++){const r=a.args[t];if(r==="-"){n.push(readFromStdin())}else if(isHttpOrHttpsUrl(r)){n.push(readFromUrl(r,e.userAgent))}else{n.push(readFromFile(r))}}getInstance(e.validator_js,e.userAgent).then((function(r){l.all(n).then((function(t){const n={};let i=false;for(let s=0;s<t.length;s++){const o=r.validateString(t[s],e.html_format);if(e.format==="json"){n[a.args[s]]=o}else{logValidationResult(a.args[s],o,e.format==="color"?true:false)}if(o.status!=="PASS"){i=true}}if(e.format==="json"){process.stdout.write(JSON.stringify(n)+"\n",(function(){process.exit(i?1:0)}))}else if(i){process.stderr.write("",(function(){process.exit(1)}))}else{process.stdout.write("",(function(){process.exit(0)}))}})).catch((function(n){process.stderr.write((e.format=="color"?t.red(n.message):n.message)+"\n",(function(){process.exit(1)}))}))})).catch((function(n){process.stderr.write((e.format=="color"?t.red(n.message):n.message)+"\n",(function(){process.exit(1)}))}))}e.main=main})();module.exports=n})();
     |                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                              ^

  at IncomingMessage.<anonymous> (../packages/next/dist/compiled/amphtml-validator/index.js:17:846)
  at process.processTicksAndRejections (../node:internal/process/task_queues:82:21)·
  Error occurred prerendering page \"/dog\". Read more: https://nextjs.org/docs/messages/prerender-error·
  Error: Could not read from https://cdn.ampproject.org/v0/validator_wasm.js - aborted
  at IncomingMessage.<anonymous> (../packages/next/dist/compiled/amphtml-validator/index.js:17:846)
  at process.processTicksAndRejections (../node:internal/process/task_queues:82:21)·
  > Export encountered errors on following paths:
  	/cat
  	/dog
  "
  at Object.toMatch (integration/amp-export-validation/test/index.test.js:28:29)

Read more about building and testing Next.js in contributing.md.

@ijjk
Copy link
Member

ijjk commented Mar 22, 2024

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary vercel/next.js 03-21-simply_applyRouterStatePatchToTree_handling Change
buildDuration 14s 14.1s N/A
buildDurationCached 7.5s 6.4s N/A
nodeModulesSize 198 MB 198 MB ⚠️ +34.2 kB
nextStartRea..uration (ms) 403ms 418ms N/A
Client Bundles (main, webpack) Overall increase ⚠️
vercel/next.js canary vercel/next.js 03-21-simply_applyRouterStatePatchToTree_handling Change
2453-HASH.js gzip 31 kB 31.3 kB ⚠️ +305 B
3304.HASH.js gzip 181 B 181 B
3f784ff6-HASH.js gzip 53.7 kB 53.7 kB
8299-HASH.js gzip 5.04 kB 5.04 kB N/A
framework-HASH.js gzip 45.2 kB 45.2 kB
main-app-HASH.js gzip 241 B 240 B N/A
main-HASH.js gzip 32.2 kB 32.2 kB N/A
webpack-HASH.js gzip 1.68 kB 1.68 kB N/A
Overall change 130 kB 130 kB ⚠️ +305 B
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js 03-21-simply_applyRouterStatePatchToTree_handling Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary vercel/next.js 03-21-simply_applyRouterStatePatchToTree_handling Change
_app-HASH.js gzip 196 B 197 B N/A
_error-HASH.js gzip 184 B 184 B
amp-HASH.js gzip 505 B 505 B
css-HASH.js gzip 324 B 325 B N/A
dynamic-HASH.js gzip 2.5 kB 2.5 kB N/A
edge-ssr-HASH.js gzip 258 B 258 B
head-HASH.js gzip 352 B 352 B
hooks-HASH.js gzip 370 B 371 B N/A
image-HASH.js gzip 4.21 kB 4.21 kB
index-HASH.js gzip 259 B 259 B
link-HASH.js gzip 2.67 kB 2.67 kB N/A
routerDirect..HASH.js gzip 314 B 312 B N/A
script-HASH.js gzip 386 B 386 B
withRouter-HASH.js gzip 309 B 309 B
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 6.57 kB 6.57 kB
Client Build Manifests
vercel/next.js canary vercel/next.js 03-21-simply_applyRouterStatePatchToTree_handling Change
_buildManifest.js gzip 481 B 484 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js 03-21-simply_applyRouterStatePatchToTree_handling Change
index.html gzip 530 B 529 B N/A
link.html gzip 541 B 541 B
withRouter.html gzip 524 B 524 B
Overall change 1.06 kB 1.06 kB
Edge SSR bundle Size
vercel/next.js canary vercel/next.js 03-21-simply_applyRouterStatePatchToTree_handling Change
edge-ssr.js gzip 95.3 kB 95.3 kB N/A
page.js gzip 3.04 kB 3.04 kB
Overall change 3.04 kB 3.04 kB
Middleware size
vercel/next.js canary vercel/next.js 03-21-simply_applyRouterStatePatchToTree_handling Change
middleware-b..fest.js gzip 625 B 624 B N/A
middleware-r..fest.js gzip 151 B 151 B
middleware.js gzip 25.5 kB 25.5 kB N/A
edge-runtime..pack.js gzip 839 B 839 B
Overall change 990 B 990 B
Next Runtimes
vercel/next.js canary vercel/next.js 03-21-simply_applyRouterStatePatchToTree_handling Change
app-page-exp...dev.js gzip 170 kB 170 kB N/A
app-page-exp..prod.js gzip 97 kB 97 kB N/A
app-page-tur..prod.js gzip 98.7 kB 98.7 kB N/A
app-page-tur..prod.js gzip 93 kB 93 kB N/A
app-page.run...dev.js gzip 144 kB 144 kB N/A
app-page.run..prod.js gzip 91.5 kB 91.5 kB N/A
app-route-ex...dev.js gzip 21.4 kB 21.4 kB
app-route-ex..prod.js gzip 15.1 kB 15.1 kB
app-route-tu..prod.js gzip 15.1 kB 15.1 kB
app-route-tu..prod.js gzip 14.8 kB 14.8 kB
app-route.ru...dev.js gzip 21 kB 21 kB
app-route.ru..prod.js gzip 14.8 kB 14.8 kB
pages-api-tu..prod.js gzip 9.55 kB 9.55 kB
pages-api.ru...dev.js gzip 9.82 kB 9.82 kB
pages-api.ru..prod.js gzip 9.55 kB 9.55 kB
pages-turbo...prod.js gzip 22.5 kB 22.5 kB
pages.runtim...dev.js gzip 23.1 kB 23.1 kB
pages.runtim..prod.js gzip 22.4 kB 22.4 kB
server.runti..prod.js gzip 51 kB 51 kB
Overall change 250 kB 250 kB
build cache Overall increase ⚠️
vercel/next.js canary vercel/next.js 03-21-simply_applyRouterStatePatchToTree_handling Change
0.pack gzip 1.57 MB 1.58 MB ⚠️ +5.17 kB
index.pack gzip 106 kB 105 kB N/A
Overall change 1.57 MB 1.58 MB ⚠️ +5.17 kB
Diff details
Diff for middleware.js

Diff too large to display

Diff for 2453-HASH.js

Diff too large to display

Diff for app-page-exp..ntime.dev.js

Diff too large to display

Diff for app-page-exp..time.prod.js

Diff too large to display

Diff for app-page-tur..time.prod.js

Diff too large to display

Diff for app-page-tur..time.prod.js

Diff too large to display

Diff for app-page.runtime.dev.js

Diff too large to display

Diff for app-page.runtime.prod.js

Diff too large to display

Commit: 42e6cc6

@ztanner ztanner marked this pull request as ready for review March 25, 2024 17:51
@ztanner ztanner force-pushed the 03-21-simply_applyRouterStatePatchToTree_handling branch from ac74fa8 to 6a3ac1f Compare March 25, 2024 19:39
@ztanner ztanner force-pushed the 03-21-prevent_router_markers_from_leaking branch from 4d248ba to 22b1baf Compare March 26, 2024 17:35
@ztanner ztanner requested a review from a team as a code owner March 26, 2024 17:35
@ztanner ztanner force-pushed the 03-21-simply_applyRouterStatePatchToTree_handling branch from 6a3ac1f to 41fcbf3 Compare March 26, 2024 17:35
@ztanner ztanner force-pushed the 03-21-prevent_router_markers_from_leaking branch from 22b1baf to d2ff9d6 Compare March 26, 2024 18:31
@ztanner ztanner force-pushed the 03-21-simply_applyRouterStatePatchToTree_handling branch from 41fcbf3 to fed2c5f Compare March 26, 2024 18:31
Base automatically changed from 03-21-prevent_router_markers_from_leaking to canary March 26, 2024 21:41
@ztanner ztanner force-pushed the 03-21-simply_applyRouterStatePatchToTree_handling branch 2 times, most recently from cf2489c to 9a4f4a4 Compare March 26, 2024 21:47
// TODO-APP: remove ''
[''],
currentTree,
treePatch
treePatch,
location.pathname
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can using the global window.location cause any weird bugs here? like race conditions with pending navigations or something?

Copy link
Member Author

@ztanner ztanner Mar 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question -- I don't think so since actions are processed sequentially - the pending navigation would have had to occur before this reducer would have had a chance to run.

But it might be safer to use state.canonicalUrl instead since that's meant to be the same value. The only reason I didn't is because it includes location.search, and I want to store only the pathname for these references. So I could probably do something like new URL(state.canonicalUrl, location.origin).pathname -- or maybe just state.canonicalUrl.split('?')[0]. Just felt kinda ugly either way 😆

### What
When a parallel segment in the current router tree is no longer "active"
during a soft navigation (ie, no longer matches a page component on the
particular route), it remains on-screen until the page is refreshed, at
which point it would switch to rendering the `default.tsx` component.
However, when revalidating the router cache via `router.refresh`, or
when a server action finishes & refreshes the router cache, this would
trigger the "hard refresh" behavior. This would have the unintended
consequence of a 404 being triggered (which is the default behavior of
`default.tsx`) or inactive segments disappearing unexpectedly.

### Why
When the router cache is refreshed, it currently fetches new data for
the page by fetching from the current URL the user is on. This means
that the server will never respond with the data it needs if the segment
wasn't "activated" via the URL we're fetching from, as it came from
someplace else. Instead, the server will give us data for the
`default.tsx` component, which we don't want to render when doing a soft
refresh.

### How
This updates the `FlightRouterState` to encode information about the URL
that caused the segment to become active. That way, when some sort of
revalidation event takes place, we can both refresh the data for the
current URL (existing handling), and recursively refetch segment data
for anything that was still present in the tree but requires fetching
from a different URL. We patch this new data into the tree before
committing the final `CacheNode` to the router.

**Note**: I re-used the existing `refresh` and `url` arguments in
`FlightRouterState` to avoid introducing more options to this data
structure that is already a bit tricky to work with. Initially I was
going to re-use `"refetch"` as-is, which seemed to work ok, but I'm
worried about potential implications of this considering they have
different semantics. In an abundance of caution, I added a new marker
type ("`refresh`", alternative suggestions welcome).

This has some trade-offs: namely, if there are a lot of different
segments that are in this stale state that require data from different
URLs, the refresh is going to be blocked while we fetch all of these
segments. Having to do a separate round-trip for each of these segments
could be expensive. In an ideal world, we'd be able to enumerate the
segments we'd want to refetch and where they came from, so it could be
handled in a single round-trip. There are some ideas on how to improve
per-segment fetching which are out of scope of this PR. However, due to
the implicit contract that `middleware.ts` creates with URLs, we still
need to identify these resources by URLs.

Fixes #60815
Fixes #60950
Fixes #51711
Fixes #51714
Fixes #58715
Fixes #60948
Fixes #62213
Fixes #61341

Closes NEXT-1845
Closes NEXT-2030
@ztanner ztanner force-pushed the 03-21-simply_applyRouterStatePatchToTree_handling branch from 649c362 to 42e6cc6 Compare March 28, 2024 12:17
@ztanner ztanner changed the title simplify applyRouterStatePatchToTree handling fix revalidation/refresh behavior with parallel routes Mar 28, 2024
@ztanner ztanner enabled auto-merge (squash) March 28, 2024 12:39
@ztanner ztanner merged commit 68de4c0 into canary Mar 28, 2024
82 checks passed
@ztanner ztanner deleted the 03-21-simply_applyRouterStatePatchToTree_handling branch March 28, 2024 12:59
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants