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.

Sign up for GitHub

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

Jump to bottom

Pass 'force' parameter for redirects #38640

Closed
wants to merge 8 commits into from
Closed

Pass 'force' parameter for redirects #38640

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fd2e5ba
pass 'force' parameter for redirects
jenae-janzen Oct 16, 2023
b581952
tests
jenae-janzen Oct 18, 2023
2c37d59
WIP: tests
jenae-janzen Oct 18, 2023
838bdd9
tests
jenae-janzen Oct 20, 2023
cbf37c5
merge main + resolve conflicts
jenae-janzen Oct 20, 2023
fd7cb06
chore: gitignore test's cache dir
pieh Oct 23, 2023
51edb37
test: update assertions for forced redirect in e2e test
pieh Oct 23, 2023
80cc170
test: make assertions a bit more confident
pieh Oct 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/gatsby/src/redux/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export interface IRedirect {
redirectInBrowser?: boolean
ignoreCase: boolean
statusCode?: HttpStatusCode
force?: boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

force is Netlify specific field - it has no meaning to Gatsby - it shouldn't be typed explicitly as this "type" of field is generally covered by [key: string]: any below (as in it will accept any additional properties that might be used by specific platform

Suggested change
force?: boolean

// Users can add anything to this createRedirect API
[key: string]: any
}
Expand Down
1 change: 1 addition & 0 deletions packages/gatsby/src/utils/adapter/manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,7 @@ function getRoutesManifest(): RoutesManifest {
(redirect.isPermanent
? HTTP_STATUS_CODE.MOVED_PERMANENTLY_301
: HTTP_STATUS_CODE.FOUND_302),
force: redirect.force,
Copy link
Contributor

Choose a reason for hiding this comment

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

Following on previous comment - as force is example of a field that Gatsby itself doesn't use, I think that a bit more "robust" would be to pass all the fields in the redirect (apart of ones that are explicit).

Something along the lines of this:

// pick fields we explicitly handle, and pass remaining ones as "platform specific" fields
const { fromPath, toPath, statusCode, isPermanent, ignoreCase, redirectInBrowser, ...platformSpecificFields } = redirect
// note that `redirectInBrowser` field will be unused - if that is `true` there is redirect mechanism in browser runtime to handle the redirects (regardless of value of this field we always want all redirects handled by "the server")

addRoute({
  path: fromPath,
  // [...] rest of the fields we already have here adjusting for the fact we destructured `redirect` already
  // so we can omit `redirect.` from most things

  // pass any unknown platform specific fields so platform adapter can use them
  ...platformSpecificFields
})

I think this change would also help with the somewhat related conditions issue not being honored (that might require additional look in https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-adapter-netlify/src/route-handler.ts but there is code that was moved from previous plugins already so it might "just work" after actually passing conditions in)

ignoreCase: redirect.ignoreCase,
headers: BASE_HEADERS,
})
Expand Down