Pass 'force' parameter for redirects

[jenae-janzen]

https://linear.app/netlify/issue/FRA-19/force-is-not-honoured

When a redirect is created, it can accept a force parameter that can trigger the redirect even if the fromPath matches a piece of content. However, this functionality isn't working with the Adapter, possibly because the parameter isn't being passed along into the routes handler.

Description

Documentation

Tests

Related Issues

Fixes FRA-19

[jenae-janzen]

In the adapter, it seems like when force is true, we just add ! to the status... what does this do?

[pieh]

In the adapter, it seems like when force is true, we just add ! to the status... what does this do?

https://docs.netlify.com/routing/redirects/redirect-options/#force-redirects Adding ! is syntax for force:true when using _redirects. The code we have in adapter is almost copy of code in previous plugin - https://github.com/netlify/gatsby-plugin-netlify/blob/c426a2daf0f53966e04ddfb912cdc0fd39b81b1c/src/create-redirects.ts#L47 so this is not net new way of handling it

[MarcL]

Can we add a unit test for this @jenae-janzen?

Other tests are here:
https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/src/utils/adapter/__tests__/manager.ts#L47

cc @pieh for guidance on this.

[pieh]

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

[pieh]

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)

[pieh]

Can we add a unit test for this @jenae-janzen?

Other tests are here: https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/src/utils/adapter/__tests__/manager.ts#L47

cc @pieh for guidance on this.

gatsby/packages/gatsby/src/utils/adapter/__tests__/fixtures/state.ts

Lines 67 to 73 in 3af35ae

	const redirects: IGatsbyState["redirects"] = [{
	fromPath: '/old-url',
	isPermanent: true,
	ignoreCase: true,
	redirectInBrowser: false,
	toPath: '/new-url'
	}]

this would need to be expanded to include a redirect that has force: true and assertion would be that this field exist and has expected value in one of the routes in routesManifest.

Setup in

gatsby/packages/gatsby/src/utils/adapter/__tests__/manager.ts

Lines 48 to 56 in 3af35ae

	it(`should return routes manifest`, () => {
	mockStoreState(stateDefault)
	process.chdir(fixturesDir)
	setWebpackAssets(new Set([`app-123.js`]))
	const routesManifest = getRoutesManifest()
	expect(routesManifest).toMatchSnapshot()
	})

is probably easiest to copy/re-use (we don't care about trailingSlash for this that other tests are checking).

The other part of unit testing ideally is done in adapter unit tests (check if ! is added) https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-adapter-netlify/src/__tests__/route-handler.ts but currently there is no test yet to replicate and we would need to create first of it's kind there.

And finally for E2E we would need to add redirect here https://github.com/gatsbyjs/gatsby/blob/master/e2e-tests/adapters/gatsby-node.ts with force: true that DOES overlap with a static asset route (we possibly need to add one - copy https://github.com/gatsbyjs/gatsby/blob/master/e2e-tests/adapters/src/pages/routes/redirect/existing.jsx and name file existing-force.jsx or something like that and make the redirect match that and then add a test that is combination of those 2

gatsby/e2e-tests/adapters/cypress/e2e/redirects.cy.ts

Lines 14 to 29 in 3af35ae

	it("should redirect from non-existing page to existing", () => {
	cy.visit(applyTrailingSlashOption(`/redirect`, TRAILING_SLASH), {
	failOnStatusCode: false,
	}).waitForRouteChange()
	.assertRoute(applyTrailingSlashOption(`/routes/redirect/hit`, TRAILING_SLASH))
	cy.get(`h1`).should(`have.text`, `Hit`)
	})
	it("should respect that pages take precedence over redirects", () => {
	cy.visit(applyTrailingSlashOption(`/routes/redirect/existing`, TRAILING_SLASH), {
	failOnStatusCode: false,
	}).waitForRouteChange()
	.assertRoute(applyTrailingSlashOption(`/routes/redirect/existing`, TRAILING_SLASH))
	cy.get(`h1`).should(`have.text`, `Existing`)
	})

- we will hit redirect that does overlap with page, but in case of force: true we expect redirect to take precedense

[jenae-janzen]

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.

[pieh]

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?

[pieh]

Suggested change

	platformSpecificFields,
	...platformSpecificFields,

otherwise this would not really match this (public) type

gatsby/packages/gatsby/src/utils/adapter/types.ts

Lines 47 to 64 in 5dbcf9e

	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

[pieh]

Closing this one in favor of #38657