Use asynchronous repo

[thedaniel]

tree-view is the best candidate for exercising the new async repo class we’re working on in atom/atom#9213

This depends on that branch of atom/atom. The plan is to merge the async repo after it proves sufficient for tree-view’s consumption, then merge this.

[thedaniel]

Hm, I only have 2 (seemingly unrelated) failures on my machine, but there are a bunch of 🔴 on travis. In any case this is starting to come together nicely.

[thedaniel]

This is green on my machine 🎉 with this change to atom - atom/atom#10352 though it seems that PR needs a little more attention before it can merge.

The specs are deeply littered with waitsfor/runs grossness, but it seemed to be the most direct route to getting them green with this big refactor. In the future we should consider breaking up the spec sections into separate files and porting them to ES6 one by one.

Also there are a few opportunities to asyncify this further but given the original intention was to just start using the async git stuff, i figured more refactoring should be done in a separate PR rather than mixing it in here.

cc @atom/feedback - this isn't quite ready to merge but let's have a review anyway 😸

[joshaber]

GitRepositoryAsync has .refreshStatusForPath which is the same as .getPathStatus, just with a clearer name.

[joshaber]

🤘 Just 1️⃣ 📝

[thedaniel]

cc: @atom/feedback @atom/core

Items for review / notable changes:

tree-view.coffee

• See big comment block at top of initialize() explaining use of a mutation observer that makes me feel not great.
• I serialized promises using Array.reduce in initialize() as we needed to expand recursively until we find (or don't find) a view for the path that's supposed to be selected on open.

directory-view.coffee

• I refactored the synchronous expand() to use a helper method called expandAsync() rather than doing the promise chaining inline, and realized that expandAsync() inside waitsForPromise is nice in tests. The old expand() returned false when done, and I kept that even though it is no longer sync, in case there were callers out there expecting that return value. Not sure if this will cause problems or if I should change the return value to a promise there as well.

directory.coffee

• updateStatus() used a lot of sync methods and got a lot more complicated using the async repo. It could use a sanity check.
• getEntries() really blew up in complexity. It was a very simple "loop over files and call sync status checks on each one" and I ultimately chose to build an array of promises checking statuses and then resolving them with Promise.all and sorting the last step of the promise chain. It seems fine, but the method doubled in size so there might be a clearer way to do it.
• I just changed reload() from sync to async, assuming that since tree-view doesn't really have a public API #itsfine.
• same with expand()

file.coffee

• updateStatus() became a 3-promise-chain async method with a bunch of conditionals, due to use of async staus checking methods

tree-view-spec.coffee

• So much ugly and repetitive conversion waitsFor and waitsForPromise :/ . In a dream world I'd love to break this up into 10 different spec files and rewrite them in ES6 using async/await but that's out of scope esp since it seems we're going to need to rewrite tree-view anyway to provide some kind of API to package developers.

[nathansobo]

😬 This is why it would be really fantastic to have a view model.

[thedaniel]

I wish we could figure out a way to rewrite tree-view incrementally without making it a giant project...

[nathansobo]

Okay, I looked at everything. It seems as reasonable as it can be for the fact that this code is woefully in need of a view model layer to separate the async complexity from DOM concerns. Asked a couple questions and suggested a couple things.

[maxbrunsfeld]

Naively, I would think we could have updateRoots return a promise that resolves after all of the directories are expanded, and do something like

@updateRoots(state.directoryExpansionStates).then =>
  if state.selectedPath
    @selectEntryForPath(state.selectedPath)
  else
    @selectEntry(@roots[0])

I assume I'm missing something; how does the Directory::onDidAddEntries code path relate to this?

[alexandernst]

Almost 2 months without any work/news on this. Is this dead?

[thedaniel]

@alexandernst not dead, just got set aside while we did some other work. The async repo support only just landed in stable a week ago so we'll need to pick this back up again soon.

[winstliu]

I don't believe we're going with the route that this PR is taking, considering nodegit is no more in Atom core.