status code return 404 unexpectedly

[michenly]

Description

Fixes (issue #)

Related #1581, #1567

Type of change

• Patch: Bug (non-breaking change which fixes an issue)
• Minor: New feature (non-breaking change which adds functionality)
• Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

• I have added a changelog entry, prefixed by the type of change noted above (Documentation fix and Test update does not need a changelog as we do not publish new version)

[michenly]

dont think this does anything really

[michenly]

whyyy, like whyyyy

[michenly]

In react router's doc. You are suppose to use req.url

The example for the url being In the request www.example.com/index.html?a=1&b=2, req.url will contain /index.html?a=1&b=2. which does not explain why pathname works

[michenly]

so to summarize what I found so far
using the following routing setup & react-router-dom@5.2.0

    <Switch>
      <Route path="/" exact component={() => <Home />} />
      <Route render={() => <NotFound />} />
    </Switch>

passing in object to StaticRouter
the router renders differently on server & on client. but only slightly.
As in the images is different but the text are the same.
Warning in browser Text content did not match. Server: "Hello World!" Client: "The page you're looking for couldn't be found"

This is currently what web uses with react-router-dom@5.1.2

passing in location.href (ie. https://site-name.com/test123) to StaticRouter
the router renders differently on server & on client fully.
When loading /, the NotFound component get loaded on server but Home get loaded on client.
Warning in browser Warning: Did not expect server HTML to contain a <div> in <div>.

This is currently what beta buddy uses with react-router-dom@5.2.0

*passin in location.pathname (ie. /test123) to StaticRouter*
This is the current fix.
But like....WHYYYYYY

[michenly]

@gmcgibbon did a little digging into react-router for me
https://github.com/ReactTraining/react-router/blob/v5.2.0/packages/react-router/modules/StaticRouter.js#L72
https://github.com/ReactTraining/history/blob/v4.10.1/modules/LocationUtils.js#L10
https://github.com/ReactTraining/history/blob/v4.10.1/modules/PathUtils.js#L24

So it looks like we do have to use either an URL object or a path string
Just need to figure out why the URL object wasn't wokring...

[gmcgibbon]

https://github.com/ReactTraining/react-router/blob/master/packages/react-router/modules/StaticRouter.js#L72

import { createLocation } from "history";
const newLocation = createLocation(location)

Logging this here I see:

{ pathname: '/',
  search: '',
  hash: '',
  [Symbol(context)]:
   URLContext {
     flags: 400,
     scheme: 'http:',
     username: '',
     password: '',
     host: 'host.name',
     port: null,
     path: [ 'test' ],
     query: null,
     fragment: null },
  [Symbol(query)]: URLSearchParams {} }

The path name gets reset to / for the initial SSR render, maybe?

[gmcgibbon]

The path name gets reset to / for the initial SSR render, maybe?

We paired on this, and that is indeed the case. We tracked it down to https://github.com/ReactTraining/history/blob/v4.10.1/modules/LocationUtils.js#L6. Since this is an outdated version of the history package, the latest version doesn't include this function, and fixing this might take some time to get released, we can instead sidestep this by simply passing in the pathname as a string.

[michenly]

What an amazing find by @gmcgibbon !!! 👏 👏 👏

To recap:
Using react-router-dom@5.2.0 + history@4.10.1 and hitting the browser at /test produce the following:

import {createLocation} from 'history';

export default function Router({location, children}: Props) {
...
  console.log('old location', location);
  console.log('new location', createLocation(location));
...
}

old location URL {
  **href: 'http://gestalt-web-application-example.myshopify.io/test',**
  origin: 'http://gestalt-web-application-example.myshopify.io',
  protocol: 'http:',
  username: '',
  password: '',
  host: 'gestalt-web-application-example.myshopify.io',
  hostname: 'gestalt-web-application-example.myshopify.io',
  port: '',
  **pathname: '/test',**
  search: '',
  searchParams: URLSearchParams {},
  hash: '' }

new location { 
   **pathname: '/',**
  search: '',
  hash: '',
  [Symbol(context)]:
   URLContext {
     flags: 400,
     scheme: 'http:',
     username: '',
     password: '',
     host: 'gestalt-web-application-example.myshopify.io',
     port: null,
     **path: [ 'test' ],**
     query: null,
     fragment: null },
  [Symbol(query)]: URLSearchParams {} }

This also explains why web & beta buddy is working ok since they are both using history@4.9.0
💯 something is happening in history@4.10.1

[michenly]

I think what make the most sense right now would be
- use pathname for StaticRouter, which is fix that require the least amount of consumer code change
edit 1 : just using pathname alone means we will be missing hash & search. Trying to see if we can build our own url object or else we need to parse href.
edit 2: building our own location object is ok, but hash value is missing from server while available in client.
edit 3: hash is not part of koa object at all. Is the idea that on the server you will never use hash?
edit 4: OK it is just a Node server thing. Server will never get the # because it is only use on client side.

• deprecate the last version of react-router as the previous fix is incorrect.
• I will follow up with react-router and see if there is a bug log

[michenly]

🛑 Final thought:

The reason why this have to happen is this line
location = { ...path }
where history is using spread operator to copy the object before using it, and we pass in URL object and copying it using spread operator means we lost all the property we actually do care about (like pathname & search).

Since the document recommend using { pathname, search, hash, state } instead of URL object.

I am updating the type to reflect the only two pieces of information we need, and building an object to pass into StaticRouter that reflect this.

This example below illustrate what it looks like using spread operator on URL object

> const url = new URL('https://gestalt-web-application-example.myshopify.io/test123');
undefined
> url
URL {
  href:
   'https://gestalt-web-application-example.myshopify.io/test123',
  origin: 'https://gestalt-web-application-example.myshopify.io',
  protocol: 'https:',
  username: '',
  password: '',
  host: 'gestalt-web-application-example.myshopify.io',
  hostname: 'gestalt-web-application-example.myshopify.io',
  port: '',
  pathname: '/test123',
  search: '',
  searchParams: URLSearchParams {},
  hash: '' }
> const foo = {...url};
undefined
> foo
{ [Symbol(context)]:
   URLContext {
     flags: 400,
     scheme: 'https:',
     username: '',
     password: '',
     host: 'gestalt-web-application-example.myshopify.io',
     port: null,
     path: [ 'test123' ],
     query: null,
     fragment: null },
  [Symbol(query)]: URLSearchParams {} }
> 

[marutypes]

Makes sense, this bug is super non-obvious so I'm glad we have a comment in the code :)