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

weird element placement behavior #214

Closed
ilhamwahabi opened this issue Jul 19, 2018 · 14 comments
Closed

weird element placement behavior #214

ilhamwahabi opened this issue Jul 19, 2018 · 14 comments

Comments

@ilhamwahabi
Copy link

ilhamwahabi commented Jul 19, 2018

after using react snap (there is no problem before), instead got this normal result:
before

with this normal structure
before


i got this weird result
after

with this weid structure
after


As you can see, the 'contact__item' is supposed inside 'contact' but instead I got it's inside 'jss20'. Its also got applied in 'home__desc'

I'm using react-reveal for animate the component and at first I think its the cause, but the result still same even when I'm remove the animation.

The fun part is, when I click the link to move to another route and back to home its become normal again. So i think the problem is with prerendered html

Already tested in latest chrome and mozilla with hard reload.

If you are curious with how i use css, and its same with another component
code

@stereobooster
Copy link
Owner

Hi @IlhamWahabiGX. It is hard to tell what exactly cause this behaviour I haven't seen anything like that before, can you try to construct minimal example where it is possible to reproduce bug. It is obvious that there is unclosed html tag somewhere, but I'm not sure why it happens.

@bfricka
Copy link

bfricka commented Jul 25, 2018

This is also happening for me. I'm trying to narrow down where the unclosed tag could be. It's been rough going so far for two reasons:

  1. React dev tools shows the correct structure
  2. None of my components have unclosed tags

@bfricka
Copy link

bfricka commented Jul 26, 2018

Update: As far as I can tell, in debugging this, these snapshots will never hydrate correctly for me. Here's what I did to get to this conclusion:

  1. My project is an ejected create-react-app + react-scripts-ts project, but it's mostly untouched. I've removed service workers / manifest and changed the output directory to dist, along w/ a few other small changes.
  2. Copied config/webpack.config.prod.js -> config/webpack.config.dev-build.js, removed UglifyJSPlugin for easier debugging and checks for NODE_ENV="production"
  3. Copied scripts/build.js -> scripts/build-dev.js, set NODE_ENV="development" and changed the config to require the dev-build config mentioned in step 2.
  4. Added a new npm script to run this stuff, and served it locally (I used lite-server, but I doubt it matters what you use as long as it handles your routes). The issue for me is on the home page anyways, so routing was irrelevant in my case.

Making these changes allowed me to more easily debug, but more importantly it uses react-dom.development.js which will throw hydration errors.

What I found was that, there's basically nothing I can do to get the app to correctly hydrate. Stepping through the hydration process, it will inevitably find children that it doesn't expect. Now, I've only been using React for a couple of weeks, so I could be way off, but I suspect it's b/c these children, when rendered w/ reactDOMServer, are supposed to get special attributes that allow them to be interpreted correctly during hydration. Basically what you'll see is that the first real DOM node that hydration finds that was actually a component in the tree will fail. At least that's what I'm seeing.

Here's what I mean:

const App = () => (
  <nav>
    <figure>Hello</figure>
    <Foo />
  </nav>
);

Hydration fails when it hits Foo because as a child, the type property of this fiber is undefined, meaning it's a component. For nav and figure, they have a type property that is their tag name and seem to be fine.

There are other, more basic hydration issues though. For example, I had to disable class and attribute sorting which are enabled by default in the options for react-snap, b/c even normal DOM nodes will fail hydration when these attributes are not 100% the same.

So all of this is concerning as it appears hydration is does not work at all in anything non-trial. However, the real question is: Why does this result in broken HTML output when hydrate throws? Every one of my pages look perfect except for the home page. None of them hydrate correctly, but they all look fine. And they are all fully interactive even with the hydration errors. I haven't looked into it, but I assume that React falls back to render when it fails hydration, so this is odd.

For example, if I load the home page, navigate to another route, and then navigate back home, everything renders correctly. React is continuing the relay even though the baton got dropped. I still haven't figured this out. However, since hydration seems infeasible in the current implementation, I can fix this by simply changing to render. Obviously, this is far from ideal. Hydrate, as I understand it, won't re-render anything, as long as it picks up all the metadata it needs.

But at least there's the crawler benefits.

NOTE: I should probably mention I'm also using Redux, and have all the state snapshot stuff setup (and it appears to work).

@stereobooster
Copy link
Owner

@bfricka it can happen that hydration issue and unclosed tag are not connected. Can you post what hydration exception saying?

Hydration fails when it hits Foo because as a child, the type property of this fiber is undefined, meaning it's a component.

Not sure what you mean. Hydration works in this example, which is pretty similar to what you post. We can create example with the same versions of libs as yours (React etc).

@bfricka
Copy link

bfricka commented Jul 26, 2018

That's a poor example b/c it's using custom scripts in a non-ejected create-react-app project. As mentioned, you need to get to a state that uses react-dom.development.js so you can see thrown hydration errors.

Currently working on some deeper debugging. I used create-react-app + the steps I outlined above to prove that there are lots of hydration warnings for trivial examples, but none of them have hit what I believe to be the more critical failure points: didNotHydrateInstance, didNotFindHydratableInstance, and similar. It's easy to replicate for invalid text nodes or classnames being out of order, but my hunch is that these aren't problematic for hydration.

The crucial thing I think I'm seeing is in tryToClaimNextHydratableInstance and tryHydrate fails. For example in one case that I'm having issues with, the fiber thinks it should be a ul when it clearly should be div. This fiber has the correct props / className, etc, but it's somehow a completely different tag. There's no conditional logic in this component that might change this dynamically, either.

But basically, when tryHydrate fails it will either insert a node by trying to hydrate the next sibling, or, if that doesn't work, it will delete the first one it tried from the parent. Here's the exact code, which seems a bit fluffy:

  if (!tryHydrate(fiber, nextInstance)) {
    // If we can't hydrate this instance let's try the next one.
    // We use this as a heuristic. It's based on intuition and not data so it
    // might be flawed or unnecessary.
    nextInstance = getNextHydratableSibling(firstAttemptedInstance);
    if (!nextInstance || !tryHydrate(fiber, nextInstance)) {
      // Nothing to hydrate. Make it an insertion.
      insertNonHydratedInstance(hydrationParentFiber, fiber);
      isHydrating = false;
      hydrationParentFiber = fiber;
      return;
    }
    // We matched the next one, we'll now assume that the first one was
    // superfluous and we'll delete it. Since we can't eagerly delete it
    // we'll have to schedule a deletion. To do that, this node needs a dummy
    // fiber associated with it.
    deleteHydratableInstance(hydrationParentFiber, firstAttemptedInstance);
  }
  hydrationParentFiber = fiber;
  nextHydratableInstance = getFirstHydratableChild(nextInstance);
}

The real question is why this discrepancy. I have a few hypotheses to try and I'll report back if I find anything useful.

Edit: Basically, if, in tryToClaimNextHydratableInstance you manage to hit the return statement in if (!isHydrating) you're not in a good place. I haven't been able to trivially reproduce this yet, but I'm still looking.

@stereobooster
Copy link
Owner

You want to say if I will eject and replace production with development I will start to see hydration errors - they exist, but I don't see it because of production build instead of development. I will try.

@bfricka
Copy link

bfricka commented Jul 26, 2018

I found the source of my particular issue. I use react-i18next for translations, and this will check to see if translations are loaded before rendering its children. I followed the render loop to the problem area and found that when it gets to this part of the tree and calls the render function on I18n, it returns null, thus creating the discrepancy.

I found that all the hydration warnings I mentioned previously are still going to be there, but hydrate itself doesn't fail unless tryHydrate fails. Additionally, the more important issue here is having a different DOM output. In my case, it's an easy fix. I can either turn off waiting ({react: {wait: false}}), or preload the initial translations.

@stereobooster
Copy link
Owner

or preload the initial translations.

You can save translations with snapSaveState.

@stereobooster
Copy link
Owner

@bfricka you are totally right about sortClassName it should be false otherwise hydration doesn't work. This is fixed in v.1.17.0. But I do not see any other issues except this one. See my test repo

@bfricka
Copy link

bfricka commented Jul 26, 2018

Yeah, I think we're on the same page. It wasn't until I stepped through everything that I saw that there are different issues associated w/ hydration. Some of these issues are minor and only throw warnings, while hydration continues (className / attribute order, text content changes, etc) successfully. Other issues, like those caused by dynamic content mismatches (i18n, and I presume redux state issues), will cause hydration to bail.

Thanks for engaging in the conversation here. I'm not sure if this addresses OP's question or not, but I would presume it has something to do w/ dynamic state missing on initial reconciliation.

Thanks for the snapSaveState suggestion. I already use this for pre-populating the redux state, but I'm not sure about using it for translations. react-i18next provider allows an the initial translations to be passed statically at run time, but it may be useful to use snapSaveState if I ever want to render different versions for each language. If I did do this, I'd have to figure out how to get puppeteer to present navigator.language / navigator.languages.

Thanks again.

@stereobooster
Copy link
Owner

I'm not sure which i18n scheme do you use, but if it is based on url e.g.

/en/faq
/de/faq

or

/en-us/faq
/de-de/faq

you can capture required translation at build time, the same way you do with Redux store.

@stereobooster
Copy link
Owner

Closing because not enough information for investigation. @IlhamWahabiGX feel free to reopen.

@camposcristian
Copy link
Contributor

camposcristian commented Oct 17, 2018

I'm having the same issue with hydration and an ejected create-react-app project. I'll try to get a simpler example emulating this. Is there any other issues that we can link here?

@hugomallet
Copy link

hugomallet commented Nov 12, 2018

Hi,

I had this kind of issue. I think it is related to your issue, my fix may help.

The reason was an incoherent pre-rendered html with the first client side render. They both should render the same tree (in order to hydrate correctly).

In my case, the render was different on the client side because of a different state set in the componentWillMount method. Replacing it with the componentDidMount method resolved the issue because the first render is now coherent with the pre-rendered html.

Hope this helps

(it may be documented in react-snap docs)

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

No branches or pull requests

5 participants