Allow redirects from same path when they have different options

[bsonntag]

Description

I'm creating a website with multiple locales and I'd like to have different routes for each locale (e.g. /en/about and /pt/about). I also want the / route to redirect to the correct locale based on the user's browser language setting.
On Netlify this can be achieved with the following _redirects file:

/  /en/  302  Language=en
/  /pt/  302  Language=pt

I'm trying to generate this using createRedirects and gatsby-netlify-plugin like the documentation explains but it's generating only one redirect rule:

/  /en/  302  Language=en

Steps to reproduce

1. Add gatsby-netlify-plugin to gatsby-config.js.
2. Add the following createPages to gatsby-node.js:

exports.createPages = ({ actions }) => {
  const { createRedirect } = actions;

  createRedirect({
    fromPath: '/',
    toPath: '/en/',
    Language: 'en',
  });
  createRedirect({
    fromPath: '/',
    toPath: '/pt/',
    Language: 'pt',
  });
}

3. Run gatsby build.

Expected result

The following _redirect file should be created on the public folder:

/  /en/  302  Language=en
/  /pt/  302  Language=pt

Actual result

The following _redirect file is created on the public folder:

/  /en/  302  Language=en

Environment

  System:
    OS: macOS 10.14.6
    CPU: (8) x64 Intel(R) Core(TM) i7-4770HQ CPU @ 2.20GHz
    Shell: 5.7.1 - /usr/local/bin/zsh
  Binaries:
    Node: 10.15.3 - ~/.nvm/versions/node/v10.15.3/bin/node
    Yarn: 1.17.3 - /usr/local/bin/yarn
    npm: 6.11.3 - ~/.nvm/versions/node/v10.15.3/bin/npm
  Languages:
    Python: 2.7.16 - /usr/local/bin/python
  Browsers:
    Chrome: 77.0.3865.90
    Firefox: 69.0.1
    Safari: 13.0.1
  npmPackages:
    gatsby: ^2.15.29 => 2.15.29 
    gatsby-image: ^2.2.24 => 2.2.24 
    gatsby-plugin-flow: ^1.1.9 => 1.1.9 
    gatsby-plugin-google-analytics: ^2.1.20 => 2.1.20 
    gatsby-plugin-manifest: ^2.2.20 => 2.2.20 
    gatsby-plugin-netlify: ^2.1.17 => 2.1.17 
    gatsby-plugin-react-helmet: ^3.1.10 => 3.1.10 
    gatsby-plugin-react-svg: ^2.1.2 => 2.1.2 
    gatsby-plugin-sharp: ^2.2.28 => 2.2.28 
    gatsby-plugin-sitemap: ^2.2.16 => 2.2.16 
    gatsby-plugin-styled-components: ^3.1.8 => 3.1.8 
    gatsby-source-filesystem: ^2.1.29 => 2.1.29 
    gatsby-transformer-sharp: ^2.2.20 => 2.2.20 

[bsonntag]

From my understanding this is happening because redirects with the same fromPath are being ignored to prevent duplicates:

gatsby/packages/gatsby/src/redux/reducers/redirects.js

Lines 8 to 13 in d766bc0

	// Add redirect only if it wasn't yet added to prevent duplicates
	if (!fromPaths.has(fromPath)) {
	fromPaths.add(fromPath)
	state.push(action.payload)
	}

Is there a way to prevent duplicates while allowing redirects with the same fromPath?

[me4502]

They're being done this way currently for performance. I'm not sure what would be the best way of achieving the same performance improvement due to the way JS's Maps and Sets work, in that they require a primitive type for comparisons. Unless the data is stringified as an object, but that adds extra JSON serialising / deserialising overhead.

[bsonntag]

Could it leave to the user to set an id (like when creating a node)? When it's not present, it could still fallback to the fromPath as an identifier.

[bsonntag]

Could it leave to the user to set an id (like when creating a node)? When it's not present, it could still fallback to the fromPath as an identifier.

I played around with this idea and have a working implementation. Should I open a PR or would something like this need an RFC first?

[me4502]

My idea was to have it be a map of lists, where the key is the path. That way it can still do decent lookups, and acts as a hash collision when it finds duplicate redirects at a path.

[bsonntag]

What would be on the lists? The toPaths? So a redirect would be a duplicate if its fromPath and toPath already exist?

[pieh]

Keys in fromPaths set don't have to be just fromPath - we could generate some key based on full redirect body (not only just on fromPath). We could use createContentDigest to generate hash from redirect object and store that

For reference - This was initial implementation:

      if (!state.some(redirect => _.isEqual(redirect, action.payload))) {
        // Add redirect only if it wasn't yet added to prevent duplicates
        return [...state, action.payload]
      }

and it was allowing for multiple redirects using same fromPath, not quite sure why this was changed (I don't mean implementation to use Set for perf - that's fine, just the behaviour one is a question)

[bsonntag]

One problem: createContentDigest returns different results for objects that are equal (when compared with _.isEqual) but have different property orders. Failing test that shows this:

  it(`returns equal content digests for objects with different property order`, () => {
    expect(createContentDigest({ a: 'a', b: 'b' })).toEqual(createContentDigest({ b: 'b', a: 'a' }))
  })

This means that the following code would generate two redirect entries with this approach:

  createRedirect({
    fromPath: '/',
    toPath: '/en/',
  });
  createRedirect({
    toPath: '/en/',
    fromPath: '/',
  });

[me4502]

When I made the PR that changed redirects to this initially, I tested by serialising the objects to string (and also manually by creating a string). It MASSIVELY impacted performance by turning the entire entry into a string.

Having a map of fromPath -> list of entire redirect entries makes the most sense, as performance won’t be impacted too much (unless you have a lot of redirects from the same path)

[bsonntag]

Oh, I see. So the check for duplicates would use _.isEqual, but it would only iterate over entries with the same fromPath.

[bsonntag]

I updated #19048 to do this.