codeintellify icon indicating copy to clipboard operation
codeintellify copied to clipboard

Allow ContextResolvers to return Observables (for async support)

Open ijsnow opened this issue 6 years ago • 0 comments

@felixfbecker you mentioned refactoring app/github/inject.tsx. I agree that it could use a lot of refactoring since it was a cluster <redacted> before because of how the old blob annortators were written and it still is now because we started working off of the old inject.tsx rather than rewriting it to fit the new model. The thing is, all of the current code host's inject.tsx are going to have the same problem.

To solve for this, I'm working on implementing a well defined interface for implementing code hosts. I came up with an interface that would be pretty easy to implement for GitHub already but with Phabricator, it is pretty hard(I'm implementing it with Phabricator as a proof of concept since it's the most hostile code host I see us working with). The only problem right now that I see is that we need more flexibility with ContextResolver. Specifically, it would be great if it could be async. This would also allow for us to give better UI feedback (in CodeViewToolbar) if we didn't have to have the commitID before even calling hoverify.

I have a WIP pr that is one solution:

- export type ContextResolver = (hoveredToken: HoveredToken) => HoveredTokenContext
+ export type ContextResolver = (hoveredToken: HoveredToken) => Observable<HoveredTokenContext> | HoveredTokenContext

Another solution that would be much easier in codeintellify but would require updating existing code hosts because its a breaking change would be just requiring it to be an observable all the time:

- export type ContextResolver = (hoveredToken: HoveredToken) => HoveredTokenContext
+ export type ContextResolver = (hoveredToken: HoveredToken) => Observable<HoveredTokenContext>

This wouldn't be too hard to update existing code hosts(GitHub and Sourcegraph) because we'd just have to wrap the current return value with of from rxjs.

ijsnow avatar Jul 30 '18 19:07 ijsnow