Skip to content
This repository has been archived by the owner on May 10, 2021. It is now read-only.

[fix] page chunk for root level catch-all is served incorrectly to client #52

Merged
merged 1 commit into from
Oct 14, 2020
Merged

[fix] page chunk for root level catch-all is served incorrectly to client #52

merged 1 commit into from
Oct 14, 2020

Conversation

lindsaylevine
Copy link
Contributor

Fixes #43

@lindsaylevine lindsaylevine force-pushed the bug/catch-all-chunk branch 3 times, most recently from 00d3007 to 0c6ab75 Compare October 10, 2020 13:19
FinnWoelm
FinnWoelm suggested changes Oct 10, 2020
View reviewed changes
Copy link
Collaborator

@FinnWoelm FinnWoelm left a comment

Choose a reason for hiding this comment

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

I think it's almost there! Most important to fix:

  • The /_next/* redirect cannot be after the root catch-all redirect (it won't ever be used AFAIK)
  • The /_next/* redirect should be /_next/* /_next/:splat 200 (or /_next/static/* /_next/static/:splat 200)

lib/steps/setupRedirects.js Outdated Show resolved Hide resolved
lib/steps/setupRedirects.js Outdated Show resolved Hide resolved
lib/steps/setupRedirects.js Outdated Show resolved Hide resolved
tests/__snapshots__/optionalCatchAll.test.js.snap Outdated Show resolved Hide resolved
@lindsaylevine lindsaylevine force-pushed the bug/catch-all-chunk branch 3 times, most recently from 1db4297 to 7f12760 Compare October 13, 2020 10:12
@FinnWoelm FinnWoelm self-requested a review October 13, 2020 20:37
FinnWoelm
FinnWoelm approved these changes Oct 13, 2020
View reviewed changes
Copy link
Collaborator

@FinnWoelm FinnWoelm left a comment

Choose a reason for hiding this comment

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

👍 Excellent!!

PS: If you want to slightly edit the comment in the helper, go for it and just merge afterwards. No need for another review :)

lib/helpers/isRootCatchAllRedirect.js Outdated Show resolved Hide resolved
lib/steps/setupRedirects.js
// Add general "no-op" redirect before the root catch-all redirect
redirects.splice(rootCatchAllIndex, 0, "/_next/* /_next/:splat 200");
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Beautiful :)

@lindsaylevine lindsaylevine merged commit 954c4a8 into netlify:main Oct 14, 2020
@lindsaylevine lindsaylevine deleted the bug/catch-all-chunk branch October 14, 2020 04:08
lindsaylevine added a commit that referenced this pull request Oct 23, 2020
@lindsaylevine
Publish v2.5.2
44d8723 
- README rebrand
- Fix: update logs to correct path constants in prepareFolders ([#58](#58))
- Fix: show experimental-serverless-trace target option in README ([#59](#59))
- Fix: x-forwarded-host is undefined on Netlify ([#54](#54))
- Fix: No-op redirect for root catch-all page chunks ([#52](#52))
- prettier pre-commit hook
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@FinnWoelm FinnWoelm FinnWoelm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Page chunk for catch-all page at root level is evaluated/rendered on the server
2 participants
@lindsaylevine @FinnWoelm