Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Rework GithubPackage model and promise management #633

Merged
merged 172 commits into from
Apr 20, 2017

Conversation

smashwilson
Copy link
Contributor

@smashwilson smashwilson commented Apr 1, 2017

I'm revisiting the way that GithubPackage manages the per-project models it tracks (Repository, Project path, and ResolutionProgress) to reduce some of the Promise soup we've got going on and ensure that the models are updated consistently.

While I'm in there, I'm attempting to streamline the way that models are initialized when the package is activated in different situations:

Remaining

  • Implement WorkdirContext
  • Implement WorkdirContextPool
  • Accept and propagate saved ResolutionProgress state
  • Update observers to accept a Repository synchronously
  • Discover a containing git working directories from a file path instead of a Project path
  • Integrate WorkdirContextPool into GithubPackage, replacing the modelsBy... maps
  • Ensure we have test coverage for active context management in GithubPackage including:
    • When activating
    • When the project paths change
    • When the active item changes
  • Use updated protocol for PaneItems
  • Wire it up for performance instrumentation

}
}
return Promise.all(readyPromises);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This replaces the .getInitialModelsPromise() and friends, but it still smells to me.

The EventHandler from #617 would be somewhat more natural, but the whole "construct a promise that will resolve the next time X occurs" pattern feels off. I'm wondering if what we really need is an EventHandler that emits events when X occurs, with an API on top of it to build Promises that resolve the next time an event is handled? Still thinking about that one.

this.initModelsPromise = initPromises.modelsPromise;
if (projectPaths.length > 0) {
this.didChangeProjectPaths(projectPaths);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When launching Atom from the command-line with a single project, the package is activated before projectPaths are populated. This was the root cause of the regression in #623: the updateActiveModels() calls in deserialize() and in didChangeProjectPaths() were happening too early and missing the project. Our test cases all set the project paths before activation, so they were passing correctly.

@smashwilson smashwilson force-pushed the aw-single-project-regression branch from 1c47701 to 909b603 Compare April 5, 2017 15:02
@smashwilson smashwilson force-pushed the aw-single-project-regression branch from 3cbfefb to 129d5e7 Compare April 7, 2017 14:12
@smashwilson
Copy link
Contributor Author

@ungb I've reproduced #656 and verified that it was fixed by the changes merged in from #654. Thanks for the report ✨

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

Successfully merging this pull request may close these issues.

3 participants