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

Pass 'force' parameter for redirects #38640

Closed
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
8 changes: 8 additions & 0 deletions e2e-tests/adapters/cypress/e2e/redirects.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,4 +54,12 @@ describe("Redirects", () => {
cy.location(`hash`).should(`equal`, `#anchor`)
cy.location(`search`).should(`equal`, `?query_param=hello`)
})
it("should force redirect ", () => {
cy.visit(applyTrailingSlashOption(`/routes/redirect/existing`, `never`), {
failOnStatusCode: false,
}).waitForRouteChange()
.assertRoute(applyTrailingSlashOption(`/routes/redirect/existing-force`, `never`))

cy.get(`h1`).should(`have.text`, `Existing Force`)
})
})
7 changes: 7 additions & 0 deletions e2e-tests/adapters/gatsby-node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,17 @@ export const createPages: GatsbyNode["createPages"] = ({ actions: { createRedire
createRedirect({
fromPath: applyTrailingSlashOption("/redirect", TRAILING_SLASH),
toPath: applyTrailingSlashOption("/routes/redirect/hit", TRAILING_SLASH),
force: true,
})
createRedirect({
fromPath: applyTrailingSlashOption("/routes/redirect/existing", TRAILING_SLASH),
toPath: applyTrailingSlashOption("/routes/redirect/hit", TRAILING_SLASH),
force: false,
})
createRedirect({
fromPath: "/routes/redirect/existing",
toPath: "/routes/redirect/existing-force",
force: true,
})

createSlice({
Expand Down
14 changes: 14 additions & 0 deletions e2e-tests/adapters/src/pages/routes/redirect/existing-force.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import * as React from "react"
import Layout from "../../../components/layout"

const ExistingForcePage = () => {
return (
<Layout>
<h1>Existing Force</h1>
</Layout>
)
}

export default ExistingForcePage

export const Head = () => <title>Existing Force</title>
18 changes: 18 additions & 0 deletions packages/gatsby-adapter-netlify/src/__tests__/route-handler.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
import fs from "fs-extra"
import { tmpdir } from "os"
import { join } from "path"
import { Route } from "gatsby/src/utils/adapter/types"
import {
injectEntries,
ADAPTER_MARKER_START,
ADAPTER_MARKER_END,
NETLIFY_PLUGIN_MARKER_START,
NETLIFY_PLUGIN_MARKER_END,
GATSBY_PLUGIN_MARKER_START,
handleRoutesManifest,
} from "../route-handler"

function generateLotOfContent(placeholderCharacter: string): string {
Expand Down Expand Up @@ -142,4 +144,20 @@ describe(`route-handler`, () => {
})
})
})

// describe(`forceRedirects`, () => {
// it(`honors the force parameter`, async () => {
// const redirects: Route = {
// path: `/old-url`,
// type: `redirect`,
// toPath: `/new-url`,
// status: 200,
// headers: [{}],
// force: true,
// conditions: { language: [`ca`, `us`] },
// }

// await handleRoutesManifest([redirects])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still trying to figure out how to test this, since handleRoutesManifest doesn't return anything to check against. Using the debugger, I can see the status being set to 200! within the method (and also parsing the conditions 🎉), but can't read the output injectEntries writes to public/_redirects from the test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's split handleRoutesManifest into 2 functions - one that does most of the work and returns the _redirect and _headers content which we can assert and the other one (probably will keep existing name) that calls the new function and then call injectEntries with results

That would allow for easy assertion (without creating some kind of spy, mocks or allowing function to actually write the files out and then read that file back).

Does that sound fine?

// })
// })
})
3 changes: 1 addition & 2 deletions packages/gatsby-adapter-netlify/src/route-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,14 +159,13 @@ export async function handleRoutesManifest(
const {
status: routeStatus,
toPath,
force,
// TODO: add headers handling
headers,
...rest
} = route
let status = String(routeStatus)

if (force) {
if (rest.force) {
status = `${status}!`
}

Expand Down
14 changes: 14 additions & 0 deletions packages/gatsby/src/utils/adapter/__tests__/fixtures/state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,20 @@ const redirects: IGatsbyState["redirects"] = [{
ignoreCase: true,
redirectInBrowser: false,
toPath: '/new-url'
}, {
fromPath: '/old-url2',
isPermanent: true,
ignoreCase: true,
redirectInBrowser: false,
toPath: '/new-url2',
force: true
}, {
fromPath: '/old-url3',
isPermanent: true,
ignoreCase: true,
redirectInBrowser: false,
toPath: '/new-url3',
force: false
}]

const functions: IGatsbyState["functions"] = [{
Expand Down
23 changes: 23 additions & 0 deletions packages/gatsby/src/utils/adapter/__tests__/manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,29 @@ describe(`getRoutesManifest`, () => {
])
)
})

it(`should respect "force" redirects parameter`, () => {
mockStoreState(stateDefault, {
config: { ...stateDefault.config },
})
process.chdir(fixturesDir)
setWebpackAssets(new Set([`app-123.js`]))

const routesManifest = getRoutesManifest()

expect(routesManifest).toEqual(
expect.arrayContaining([
expect.objectContaining({
path: `/old-url2`,
platformSpecificFields: { force: true },
}),
expect.objectContaining({
path: `/old-url3`,
platformSpecificFields: { force: false },
}),
])
)
})
})

describe(`getFunctionsManifest`, () => {
Expand Down
21 changes: 16 additions & 5 deletions packages/gatsby/src/utils/adapter/manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -458,17 +458,28 @@ function getRoutesManifest(): RoutesManifest {

// redirect routes
for (const redirect of state.redirects.values()) {
const {
fromPath,
toPath,
statusCode,
isPermanent,
ignoreCase,
redirectInBrowser,
...platformSpecificFields
} = redirect

addRoute({
path: redirect.fromPath,
path: fromPath,
type: `redirect`,
toPath: redirect.toPath,
toPath: toPath,
status:
redirect.statusCode ??
(redirect.isPermanent
statusCode ??
(isPermanent
? HTTP_STATUS_CODE.MOVED_PERMANENTLY_301
: HTTP_STATUS_CODE.FOUND_302),
ignoreCase: redirect.ignoreCase,
ignoreCase: ignoreCase,
headers: BASE_HEADERS,
platformSpecificFields,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
platformSpecificFields,
...platformSpecificFields,

otherwise this would not really match this (public) type

export interface IRedirectRoute extends IBaseRoute {
type: `redirect`
/**
* The redirect should happen from `path` to `toPath`.
*/
toPath: string
/**
* HTTP status code that should be used for this redirect.
*/
status: HttpStatusCode
ignoreCase?: boolean
/**
* HTTP headers that should be used for this redirect.
* @see http://www.gatsbyjs.com/docs/how-to/previews-deploys-hosting/headers/
*/
headers: IHeader["headers"]
[key: string]: unknown
}

(technically it does matches it, but this is more that shape of redirect is more about having platform specific fields as top level fields and not have actual platformSpecificFields field that is object with platform specific fields.

Also with this as-is the current code in this PR in packages/gatsby-adapter-netlify/src/route-handler.ts wouldn't actually work as it does expect force field to be top-level field and not nested under platformSpecificFields

This change will need some adjustment in test added to packages/gatsby/src/utils/adapter/tests/manager.ts

})
}

Expand Down