Wait for Etch component to render before consuming

[Alhadis]

Fixes atom/atom#16133. Notes copied from my commit:

The process.nextTick hack assumed that ALL Etch components would their child elements on the same thread. As it turns out, there can be a delay between constructing a VirtualDOM node and when it's attached to the DOM tree.

@nathansobo I wasn't sure how to go about adding tests for this, since it would involve a fixtures folder full of dozens of files and all with numerous matches... Would you be okay with that? I don't think the current tests are long enough to delay rendering until the user scrolls far enough.

/cc @alexdevero, @50Wliu

[nathansobo]

Okay, I thought I'd understand it when I saw the code but I don't.

Can you provide a bit of context for what the process.nextTick hack was solving? This code seems complex and I want to make sure I fully understand the problem it is solving.

[Alhadis]

The problem is that there are no DOM references visible in the Etch code that's used to generate a new Project Find Result:

$.li(
  {
    key: this.filePath,
    dataset: {path: this.filePath},
    className: [
      'path',
      'list-nested-item',
      isPathSelected ? 'selected' : '',
      this.isExpanded ? '' : 'collapsed'
    ].join(' ').trim()
  },

  $.div({ref: 'pathDetails', className: 'path-details list-item'},
    $.span({className: 'disclosure-arrow'}),
    $.span({
      ref: 'icon',
      className: iconClass + ' icon',
      dataset: {name: path.basename(this.filePath)}
    }),
    $.span({className: 'path-name bright'},
      relativePath
    ),
    $.span({ref: 'description', className: 'path-match-number'},
      `(${this.matches.length} match${this.matches.length === 1 ? '' : 'es'})`
    )
  ),

  this.renderList()
)

I searched through the properties above, but there are no element references (which I assume are generated from inside Etch's core code, not the find-and-replace package):

So I resorted to async execution instead. The process.nextTick() could've easily been a setImmediately() or a setTimeout().

[Alhadis]

Now, the reason this suddenly became a problem is that, with very long lists of search results, Etch elements were being created "ahead of time", it seems. That is, created long before they were attached or fully initialised. Which means the view.refs.icon property gets defined only after the user's scrolled down far enough to see more search results.

[nathansobo]

If you extracted these list items as their own component, then you'd have access to their element on construction. Then you could go ahead and pass your element to the appropriate service.

Maybe that's what you were looking for in your original question this morning? Apologies for missing the point of your question if that's the case.

[nathansobo]

Okay, looking more closely at the code, I can see what's going on. I don't think we should be passing this to the icons service inside of render. Sorry, I should have caught that on the initial review. I need to get better at reviewing.

That method is intended to be free of side effects like the ones that are performed in getIconClasses to create and assign the iconDisposable. Now I need to backfill some questions I should have asked before... Why can't we just return a class there as a constant and be done with it? Because the service can change the class in the future?

If that's true, it might be better to just update the class imperatively in result view after construction or when the etch.update promise resolves rather than weaving this into render. Then you'll be guaranteed that the content has actually rendered once.

[Alhadis]

Why can't we just return a class there as a constant and be done with it? Because the service can change the class in the future?

The class can change, yes. The service also needs to know what to change when the class needs updating.

If that's true, it might be better to just update the class imperatively in result view after construction or when the etch.update promise resolves rather than weaving this into render.

I'll have a crack at updating the class imperatively in result view, then...

[nathansobo]

@Alhadis Heads up, I reverted the original PR so we can regroup and come up with a better solution. We still want these changes though.

[Alhadis]

Closing this; will submit another with a more appropriate implementation, as per your recommendations. :)