Preact: Dev Tools weird behaviour

[ForsakenHarmony]

Entry

Initial state of dev tools
after changing route

bundled with rollup

---

Steps to reproduce:

• Create a Router with Routes
• Go into the Dev Tools

---

JSFiddle

[developit]

Did we determine that this was related to rollup?

[ForsakenHarmony]

I've switched back to webpack and still have the same issue

[robertknight]

Sorry I haven't gotten around to this yet. I'll try to take a look in the next couple of days.

[robertknight]

OK, after some initial investigation it appears that the duplicate nodes happen without preact-router. This minimal test case also produces incorrect behavior:

/** @jsx h */

const { h, render } = require('preact');
require('preact/devtools');

const Index = () => (
  <main>
   DevTools Test 
  </main>
);

const App = () =>
  <Index path='/'/>

render(<App />, document.body);

Result:

[developit]

Maybe something to do with compositional components then, since both Router and your repro use it. Good find. I'll see about pushing my devtools changes branch - I wasn't able to get it working (changed too much at once) but it might be a source of ideas.

[robertknight]

So far what I've found is that the method of testing whether a newly mounted component is a root or not is wrong. The current check looks at whether the parent Element of a component's base Element was rendered by preact or not. This fails in the case where the root component renders only one child which is also a composite.

This fixes the minimal test case above: https://github.com/developit/preact/compare/master...robertknight:devtools-nested-composite-root?expand=1

However it doesn't yet get the example in the original issue working properly. I'm still looking into that.

[developit]

Nice - good find. Maybe combine the two checks?

return !component._parentComponent && !(component.base.parentNode && component.base.parentNode[ATTR_KEY]);

[robertknight]

After some further investigation, it looks like the problem may be due to the order in which changes are reported to the devtools. With the default ordering, a change of route results in the update to the Router component being reported before the mounting of the child component for the active route.

Adding logic to sort updates so that mounts of new components in a given batch of updates are reported before updates to existing components, including the parent Router in this case, resolves the problem. I need to do a bit more digging to verify that this is actually the problem though.

[robertknight]

So the problem is definitely due to the difference in ordering of lifecycle callbacks for children and parents when an update occurs. Given a structure like this:

<Parent>
  <ChildA/>
  <ChildB/>
</Parent>

Where <Parent> renders ChildA or ChildB depending on its current state/props, an update to Parent in React that changes the child will result in the lifecycle hooks being invoked in this order:

1. The new child's componentDidMount
2. The parent's componentDidUpdate

In Preact, the hooks are invoked the other way around, breaking the devtools expectations. There are a couple of ways we can fix this:

1. Decide that Preact's lifecycle hook ordering should match React here, and modify the diff code. This will fix this issue as a side effect.
2. Keep the current ordering, and modify the devtools bridge to reorder the events before reporting them to the React Dev Tools.

Thoughts?

[developit]

@robertknight Interesting. Given that Preact fires componentDidMount hooks last (via a stack to defer until the end of diff()), I'm not sure how we'd reverse those TBH. Very open to making them fire in the same order as React, just it might be super difficult or costly which might make it harder to justify.

Not totally related, but I found a way to rewrite the Pure Functional Components flow that pretty much removes all special-casing. In doing so, I was able to completely remove the wrapping fix you had to add to the devtools hook, since that's basically what is happening behind the scenes now! 🎉

[robertknight]

I'm not sure how we'd reverse those TBH. Very open to making them fire in the same order as React, just it might be super difficult or costly which might make it harder to justify.

OK. I'll have a look. Unless we have a compelling reason to respect React's lifecycle hook ordering here then I'm fine with option (2) above for now. In order to do that, the devtools would need a way of knowing when a batch of updates has occurred. I'm thinking of doing that by adding a hook, eg. afterRender that is called once a render cycle has completed and all lifecycle hooks have been triggered. SGTY?

In doing so, I was able to completely remove the wrapping fix you had to add to the devtools hook, since that's basically what is happening behind the scenes now!

Great! - That simplifies things somewhat :)

[developit]

I wonder if there's an existing place to plug into actually...

[robertknight]

I've had a stab at addressing this by queuing both update and mount callbacks and executing all of them in-order at the end of the render cycle: https://github.com/developit/preact/compare/master...robertknight:update-callback-order?expand=1 . FWIW React does the same thing.

Gzip size (master): 3934
Gzip size (this branch): 3994

Thoughts?

[developit]

@robertknight nice, thanks for heading that up. I'm worried about that size bump, but maybe there are tweaks we can make to get it smaller? Instead of assigning prevProps etc to locals, maybe assign them as underscore properties on the component instance and switch back to just storing component in updates? (not sure if it'd be smaller)

I hadn't thought of the issue of deferring the previous values there, that makes this a bit more complex and kinda makes me wonder if it's too much. Not sure.