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

Comparing a component's "type" to the component is not producing expected result #3486

Closed
bradwestfall opened this Issue Jan 11, 2018 · 23 comments

Comments

Projects
None yet
3 participants
@bradwestfall
Copy link

bradwestfall commented Jan 11, 2018

Description

I have a few components that rely on React.Children.map to eventually compare child.type to the expected child component. This code works fine in all my React projects that use ordinary react (like Create React App), but doesn't work in Gatsby or NextJS (perhaps something to do with SSR) All my code is on the same version of React and Node (listed below)

Here is a basic example of a "compound components" pattern where <Foo> expects <Bar> as it's child. In my CRA apps I get "Bar: world" as the end result (which is correct), but with Gatsby I get "not same" which shows a difference in how the two environments evaluate this code child.type === Bar

const Foo = (props) => {
  const newChildren = React.Children.map(props.children, child => {
    return child.type === Bar
      ? React.cloneElement(child, { hello: 'world' })
      : <div>not same</div>
  })
  return <div>{newChildren}</div>
}

const Bar = (props) => (
  <div>bar: {props.hello}</div>
)

const App = () => (
  <Foo><Bar /></Foo>
)

In a quick twitter chat with gatsby, I was asked if refreshing made a difference, it does not

Environment

Gatsby version: 1.1.28
Node.js version: 8.2.1
Operating System: macOS High Siera

Actual result

child.type is not === to what it should be equal to

Expected behavior

It should be equal

Steps to reproduce

Copy the above demo code and implement with <Foo><Bar /></Foo>

@bradwestfall

This comment has been minimized.

Copy link

bradwestfall commented Jan 11, 2018

If I add some console logs like this:

const Foo = (props) => {
  const newChildren = React.Children.map(props.children, child => {
    console.log(child.type)
    console.log(Bar)
    return child.type === Bar
      ? React.cloneElement(child, { hello: 'world' })
      : <div>not same</div>
  })
  return <div>{newChildren}</div>
}

Notice the output suggests they are the same "component", but again the truthiness is false leading to the "not same" output

screen shot 2018-01-11 at 1 50 57 pm

@KyleAMathews

This comment has been minimized.

Copy link
Contributor

KyleAMathews commented Jan 22, 2018

Ah, think I figured it out

We're cloning the children prop.

@KyleAMathews

This comment has been minimized.

Copy link
Contributor

KyleAMathews commented Jan 22, 2018

We should probably just pass it in directly then yeah?

@bradwestfall

This comment has been minimized.

Copy link

bradwestfall commented Jan 23, 2018

@KyleAMathews I don't fully understand the context of the code you showed, but do you mean something like this to preserve children?:

return createElement(ComponentRenderer, {
  page: true,
  ...props,
  children: props.children
})
@KyleAMathews

This comment has been minimized.

Copy link
Contributor

KyleAMathews commented Jan 23, 2018

Yeah exactly. When you have a chance, could you test that to make sure it works and then PR the change?

@bradwestfall

This comment has been minimized.

Copy link

bradwestfall commented Jan 23, 2018

@KyleAMathews I followed all the steps to use gatsby-dev-cli, and I'm fairly certain it's working -- when I make changes to my cloned repo in that file, I can see the same file in my site get the change, I have the watch running etc. However, when I do a refresh on the site I don't see changes. I tried doing this console log to prove this function was every running:

render: routeProps => {
  attachToHistory(routeProps.history)
  const props = layoutProps ? layoutProps : routeProps

  console.log('here')

  if (loader.getPage(props.location.pathname)) {
    return createElement(ComponentRenderer, {
      page: true,
      ...props
    })
  } else {
    return createElement(ComponentRenderer, {
      page: true,
      location: { pathname: `/404.html` },
    })
  }
}

...and I didn't get a log in dev tools. Also made the props change because I'm not sure if this is a SSR method, but that didn't work. I'm sure I'm doing something wrong here with dev work in Gatsby, but this is a needle in a haystack situation for me since I'm pretty unfamiliar with Gatsby setup. Any pointers?

@KyleAMathews

This comment has been minimized.

Copy link
Contributor

KyleAMathews commented Jan 23, 2018

Yeah, making changes to production-app.js is a bit tricky as it's only used for "production" e.g. when you run gatsby build. root.js and app.js are used during development. Try building your site and then see if your logs show up.

@bradwestfall

This comment has been minimized.

Copy link

bradwestfall commented Jan 23, 2018

@KyleAMathews we're making progress, made a production build then cd 'd to public and ran http-server to run it, I get the console log, and the problem with <Foo><Bar /></Foo> is fixed - it outputs "bar: world" instead of "not same". So that's great, but I didn't have to change anything to get it to work, so there's a discrepancy between dev and production. Just for the heck of it, I changed this line of root.js for dev:

              if (pageResources && pageResources.component) {
                return createElement(ComponentRenderer, {
                  key: `normal-page`,
                  page: true,
                  ...props,
                  children: props.children,
                  pageResources,
                })
              } else {

... to include the children: props.children idea, and it didn't work for dev mode

@KyleAMathews

This comment has been minimized.

Copy link
Contributor

KyleAMathews commented Jan 23, 2018

Awesome!

That's surprising the same change didn't work in dev. Double check your change is being applied? The children component must be being cloned elsewhere still.

@bradwestfall

This comment has been minimized.

Copy link

bradwestfall commented Jan 23, 2018

I probably didnt say the last message clear enough, the only thing I did in production was the console log. So production always works without any changes but development doesn't work

@KyleAMathews

This comment has been minimized.

Copy link
Contributor

KyleAMathews commented Jan 23, 2018

Oh ok, gotcha. So mystery is still unsolved

@bradwestfall

This comment has been minimized.

Copy link

bradwestfall commented Jan 29, 2018

I just haven't had time to keep looking at this yet, but I will soon I hope

@scottyeck

This comment has been minimized.

Copy link
Contributor

scottyeck commented Feb 7, 2018

FWIW, we can confirm @bradwestfall 's observation that this behavior currently only affects the local gatsby develop environment. We're going to do some digging. If anybody's got thoughts on a path forward, we'd love to hear. 😎

@bradwestfall

This comment has been minimized.

Copy link

bradwestfall commented Feb 7, 2018

I'm really sorry I've been absent from this thread, I still really want to get this solved but I got pulled away on other projects. But bottom line is that I can't use Gatsby at the moment with some important components I have because of this. I hope to look at it again soon but I'm very unfamiliar with Gatsby internals so it's hard to know where to start

if this helps, I get the same problem with NextJS. So there must be some common trait with these SSR tools, if that gives you any clues. I've never done anything SSR related except for using Next and Gatsby, and also never had this problem before with components that compare types

@scottyeck

This comment has been minimized.

Copy link
Contributor

scottyeck commented Feb 7, 2018

I'm also not able to spend the time on this that I wanted to unfortunately. For anyone who's interested, I set up @bradwestfall 's test case in an example repo here: https://github.com/fabrictech/gatsby/tree/example-check-types/examples/check-types

@KyleAMathews

This comment has been minimized.

Copy link
Contributor

KyleAMathews commented Feb 8, 2018

Ahh looks like react-hot-loader gaearon/react-hot-loader#304

@scottyeck

This comment has been minimized.

Copy link
Contributor

scottyeck commented Feb 8, 2018

@KyleAMathews What are your thoughts on auto-injecting that babel-plugin to fix this in the develop environment? Happy to PR if you think it makes sense, but I can go either way on it.

@KyleAMathews

This comment has been minimized.

Copy link
Contributor

KyleAMathews commented Feb 8, 2018

@scottyeck which one? Seems like the reports are mixed on the babel plugin mentioned in the issue + that'd only help with imports which wouldn't solve the reproduction you added above.

@bradwestfall

This comment has been minimized.

Copy link

bradwestfall commented Feb 8, 2018

@scottyeck @KyleAMathews As I'm reading through that thread, I saw that one option is to do child.type === <Bar />.type instead of child.type === Bar. Turns out that is also mentioned in their v3 docs. For v4, they plan on providing a helper function instead

I guess this is complex and I see why that might be their best solution, but it's a little funky for me because my app doesn't depend on react-hot-loader directly for the helper function. So I tried child.type === <Bar />.type and it works for me now in Gatsby.

Although I hope you can still get some sort of Babel plugin to work so users of Gatsby don't have to know all this ;)

@scottyeck

This comment has been minimized.

Copy link
Contributor

scottyeck commented Feb 8, 2018

Thanks for following up everybody. Yucko.

@KyleAMathews Yeah - I'd agree that the babel-plugin doesn't quite feel like the way to go. Regardless, thanks a TON for doing the research and tracking this down. Made life a heck of a lot easier.

@bradwestfall Yup - we're doing the same as a workaround for the time being. As it mentions in the docs, this is addressed in react-hot-loader's v4 release, though I'm guessing we're still quite a ways away from that as upgrading webpack should be the priority IMO, and @KyleAMathews and @jquense have been super hard at work there. Don't wanna do anything to derail that effort. That said, you bring up a good point - as react-hot-loader is something that gatsby relies on, we should maybe also abstract the funk away. That way when we finally do upgrade, there's a clear path forward.

We could publish a utility package - something like gatsby-are-components-equal that would act in the same way react-hot-loader's v4 check does but perform that check on an env-specific basis. In gatsby develop environments, it'd check against child.type === <Bar/>.type, but in gatsby build environments, it'd check against child.type === Bar. I don't love the idea of build logic creeping its way in like that, but it'd at least improve the issue a bit. FWIW, this is how we've structured our workaround for now.

I'm happy to take the lead on this guy if we feel it's helpful.

@bradwestfall

This comment has been minimized.

Copy link

bradwestfall commented Feb 8, 2018

@scottyeck as far as having the two ways for dev and production, I'm just going to short circuit it:

child.type === Bar || child.type === <Bar />.type

Thank you all for your help tracking this down

@KyleAMathews

This comment has been minimized.

Copy link
Contributor

KyleAMathews commented Feb 8, 2018

@scottyeck that looks great. A little utility lib + a docs page explaining what the heck is going on that we can direct people to should suffice for now.

Glad it's sorta working now! :-)

@KyleAMathews

This comment has been minimized.

Copy link
Contributor

KyleAMathews commented Sep 8, 2018

Due to the high volume of issues, we're closing out older ones without recent activity. Please open a new issue if you need help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment