Conversation
|
|
||
| for (const codeHost of codeHosts) { | ||
| const check = codeHost.check() | ||
| const checking = check instanceof Promise ? check : Promise.resolve(check) |
There was a problem hiding this comment.
You can call Promise.resolve() on maybe-Promises (it's made for that use case), and directly await the return
| return subscriptions | ||
| } | ||
|
|
||
| async function injectCodeIntelligenceToCodeHosts(codeHosts: CodeHost[]): Promise<Subscription> { |
There was a problem hiding this comment.
An async function that returns Subscriptions on first sight seems like a weird mix of patterns to me
There was a problem hiding this comment.
Should I not make this an async function?
There was a problem hiding this comment.
It can be (it looks like it does only side effects)
I just thought the returned Subscription was weird because if it is to cancel the operation, the operation already happened when the Promise resolves. But I see that it actually represents the subscription for hoverify etc? In that case this makes sense. Just some documentation would help explain what the return value actually is
| * Cast a Node to an HTMLElement if it has a classList. This should not be used | ||
| * if you need 100% confidence the Node is an HTMLElement. | ||
| */ | ||
| function naiveCheckIsHTMLElement(node: Node): node is HTMLElement { |
There was a problem hiding this comment.
What's the problem with instanceof HTMLElement?
There was a problem hiding this comment.
Well, tbh didn't really think about it. I get why Node, Element, and HTMLElement all exists as separate types but they do confuse me sometimes.
| // as a prop, but TS complains that it does not | ||
| // exist. | ||
| if ((entry as any).isIntersecting) { | ||
| intersectingElements.next(entry.target as HTMLElement) |
There was a problem hiding this comment.
Instead of Subjects, prefer the Observable constructor
There was a problem hiding this comment.
This is probably related to your comment below but I found it quite hard to implement this in another way. I only ever want to create on IntersectionObserver so I put it above the function that gets called for everything that's piped through but it needs to be used there so I created a Subject so data could be piped into it.
There was a problem hiding this comment.
I tried using the observable constructor but ended up here
| return (codeViews: Observable<ResolvedCodeView>) => | ||
| new Observable<ResolvedCodeView>(observer => { | ||
| codeViews.subscribe(({ codeView, ...rest }) => { | ||
| intersectionObserver.observe(codeView) |
There was a problem hiding this comment.
It seems wrong to have the intersectionObserver be shared between all Observable subscriptions at this layer - if the intend is to share, the share() operator should accomplish that
There was a problem hiding this comment.
To be honest, I'm not sure how to use that in place of this subscription here. That sounds reasonable. Could you give me some guidance?
src/libs/github/code_intelligence.ts
Outdated
| toolbarButtonProps, | ||
| } | ||
|
|
||
| const resolveCodeView = (elem: HTMLElement) => { |
| import { resolveDiffFileInfo } from './file_info' | ||
| import { createCodeViewToolbarMount, parseURL } from './util' | ||
|
|
||
| const toolbarButtonProps = { |
| map(({ codeView, ...rest }) => { | ||
| const { headFilePath, baseFilePath } = getDeltaFileName(codeView) | ||
| if (!headFilePath) { | ||
| throw new Error('cannot determine file path') |
There was a problem hiding this comment.
According to the type this never happens, in what case does it happen?
| map(({ codeView, ...rest }) => { | ||
| const diffResolvedRev = getDiffResolvedRev() | ||
| if (!diffResolvedRev) { | ||
| throw new Error('cannot determine delta info') |
There was a problem hiding this comment.
In what case does this happen? This error message is rather unhelpful, should getDiffResolvedRev maybe throw the error instead?
There was a problem hiding this comment.
There isn't an error as far as I know. This piece was just ported over from existing logic. Would probably be worth looking into at some point.
| }, | ||
| ] | ||
|
|
||
| function checkIsPhabricator(): Promise<boolean> | boolean { |
There was a problem hiding this comment.
Could this just always return a Promise for simplicity? You could just make it an async function
|
@chrismwendt last TODO is to get extensions stuff in here. We could merge this in behind a feature flag defaulted to off and then follow up with that |
|
This is now behind a feature flag. We don't need to worry about blowing away extensions stuff before merging this in. cc @chrismwendt |
|
🎉 This PR is included in version 1.15.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
4 participants
This PR changes GitHub to use the new generic way of injecting code intelligence.
A couple TODOs:
Talk with @chrismwendt about what I blew away in terms of Extensions and how to get it added backTesting plan:
Go through the regular flows of using
I have tested on: