Skip to content
This repository was archived by the owner on Jan 22, 2019. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/browser/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,12 @@ export interface PhabricatorMapping {
*/
export interface FeatureFlags {
newTooltips: boolean
newInject: boolean
}

export const featureFlagDefaults: FeatureFlags = {
newTooltips: true,
newInject: false,
}

// TODO(chris) Switch to Partial<StorageItems> to eliminate bugs caused by
Expand Down
11 changes: 10 additions & 1 deletion src/extension/scripts/inject.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
import { featureFlags } from '../../shared/util/featureFlags'

import { injectBitbucketServer } from '../../libs/bitbucket/inject'
import { injectCodeIntelligence } from '../../libs/code_intelligence'
import { injectGitHubApplication } from '../../libs/github/inject'
import { injectPhabricatorApplication } from '../../libs/phabricator/app'
import { injectSourcegraphApp } from '../../libs/sourcegraph/inject'
Expand All @@ -34,7 +35,7 @@ function injectApplication(): void {

const href = window.location.href

const handleGetStorage = (items: StorageItems) => {
const handleGetStorage = async (items: StorageItems) => {
if (items.disableExtension) {
return
}
Expand Down Expand Up @@ -96,6 +97,14 @@ function injectApplication(): void {
setSourcegraphUrl(sourcegraphServerUrl)
injectBitbucketServer()
}

if (isGitHub || isPhabricator) {
if (await featureFlags.isEnabled('newInject')) {
const subscriptions = await injectCodeIntelligence()
window.addEventListener('unload', () => subscriptions.unsubscribe())
}
}

setUseExtensions(items.useExtensions === undefined ? false : items.useExtensions)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,24 +15,78 @@ import { HoverMerged } from '@sourcegraph/codeintellify/lib/types'
import { toPrettyBlobURL } from '@sourcegraph/codeintellify/lib/url'
import * as React from 'react'
import { render } from 'react-dom'
import { Observable, of, Subject, Subscription } from 'rxjs'
import { filter, map, withLatestFrom } from 'rxjs/operators'
import { animationFrameScheduler, Observable, of, Subject, Subscription } from 'rxjs'
import { filter, map, mergeMap, observeOn, withLatestFrom } from 'rxjs/operators'

import { createJumpURLFetcher } from '../../shared/backend/lsp'
import { lspViaAPIXlang } from '../../shared/backend/lsp'
import { ButtonProps, CodeViewToolbar } from '../../shared/components/CodeViewToolbar'
import { eventLogger, sourcegraphUrl } from '../../shared/util/context'
import { githubCodeHost } from '../github/code_intelligence'
import { phabricatorCodeHost } from '../phabricator/code_intelligence'
import { findCodeViews } from './code_views'

/**
* Defines a type of code view a given code host can have. It tells us how to
* look for the code view and how to do certain things when we find it.
*/
export interface CodeView {
/** A selector used by `document.querySelectorAll` to find the code view. */
selector: string
/** The DOMFunctions for the code view. */
dom: DOMFunctions
/**
* Finds or creates a DOM element where we should inject the
* `CodeViewToolbar`. This function is responsible for ensuring duplicate
* mounts aren't created.
*/
getToolbarMount?: (codeView: HTMLElement, part?: DiffPart) => HTMLElement
/**
* Resolves the file info for a given code view. It returns an observable
* because some code hosts need to resolve this asynchronously. The
* observable should only emit once.
*/
resolveFileInfo: (codeView: HTMLElement) => Observable<FileInfo>
/**
* In some situations, we need to be able to adjust the position going into
* and coming out of codeintellify. For example, Phabricator converts tabs
* to spaces in it's DOM.
*/
adjustPosition?: PositionAdjuster
/** Props for styling the buttons in the `CodeViewToolbar`. */
toolbarButtonProps?: ButtonProps
}

export type CodeViewWithOutSelector = Pick<CodeView, Exclude<keyof CodeView, 'selector'>>

export interface CodeViewResolver {
selector: string
resolveCodeView: (elem: HTMLElement) => CodeViewWithOutSelector
}

/** Information for adding code intelligence to code views on arbitrary code hosts. */
export interface CodeHost {
/**
* The name of the code host. This will be added as a className to the overlay mount.
*/
name: string

/**
* The list of types of code views to try to annotate.
*/
codeViews: CodeView[]
codeViews?: CodeView[]

/**
* Resolve `CodeView`s from the DOM. This is useful when each code view type
* doesn't have a distinct selector for
*/
codeViewResolver?: CodeViewResolver

/**
* Checks to see if the current context the code is running in is within
* the given code host.
*/
check: () => Promise<boolean> | boolean
}

export interface FileInfo {
Expand All @@ -55,7 +109,6 @@ export interface FileInfo {
* The revision the code view is at. If a `baseRev` is provided, this value is treated as the head rev.
*/
rev?: string

/**
* The repo bath for the BASE side of a diff. This is useful for Phabricator
* staging areas since they are separate repos.
Expand All @@ -78,31 +131,6 @@ export interface FileInfo {
baseHasFileContents?: boolean
}

/**
* Defines a type of code view a given code host can have. It tells us how to
* look for the code view and how to do certain things when we find it.
*/
export interface CodeView {
/** A selector used by `document.querySelectorAll` to find the code view. */
selector: string
/** The DOMFunctions for the code view. */
dom: DOMFunctions
/** Finds or creates a DOM element where we should inject the `CodeViewToolbar`. */
getToolbarMount?: (codeView: HTMLElement, part?: DiffPart) => HTMLElement
/** Resolves the file info for a given code view. It returns an observable
* because some code hosts need to resolve this asynchronously. The
* observable should only emit once.
*/
resolveFileInfo: (codeView: HTMLElement) => Observable<FileInfo>
/** In some situations, we need to be able to adjust the position going into
* and coming out of codeintellify. For example, Phabricator converts tabs
* to spaces in it's DOM.
*/
adjustPosition?: PositionAdjuster
/** Props for styling the buttons in the `CodeViewToolbar`. */
toolbarButtonProps?: ButtonProps
}

/**
* Prepares the page for code intelligence. It creates the hoverifier, injects
* and mounts the hover overlay and then returns the hoverifier.
Expand Down Expand Up @@ -196,41 +224,43 @@ function initCodeIntelligence(codeHost: CodeHost): { hoverifier: Hoverifier } {
* ResolvedCodeView attaches an actual code view DOM element that was found on
* the page to the CodeView type being passed around by this file.
*/
export interface ResolvedCodeView extends CodeView {
export interface ResolvedCodeView extends CodeViewWithOutSelector {
/** The code view DOM element. */
codeView: HTMLElement
}

function findCodeViews(codeViewInfos: CodeView[]): Observable<ResolvedCodeView> {
return new Observable<ResolvedCodeView>(observer => {
for (const info of codeViewInfos) {
const elements = document.querySelectorAll<HTMLElement>(info.selector)
for (const codeView of elements) {
observer.next({ ...info, codeView })
}
}
})
}
function handleCodeHost(codeHost: CodeHost): Subscription {
const { hoverifier } = initCodeIntelligence(codeHost)

export function injectCodeIntelligence(codeHostInfo: CodeHost): Subscription {
const { hoverifier } = initCodeIntelligence(codeHostInfo)
const subscriptions = new Subscription()

return findCodeViews(codeHostInfo.codeViews).subscribe(
({ codeView, dom, resolveFileInfo, adjustPosition, getToolbarMount, toolbarButtonProps }) =>
resolveFileInfo(codeView).subscribe(info => {
subscriptions.add(
of(document.body)
.pipe(
findCodeViews(codeHost),
mergeMap(({ codeView, resolveFileInfo, ...rest }) =>
resolveFileInfo(codeView).pipe(map(info => ({ info, codeView, ...rest })))
),
observeOn(animationFrameScheduler)
)
.subscribe(({ codeView, info, dom, adjustPosition, getToolbarMount, toolbarButtonProps }) => {
const resolveContext: ContextResolver = ({ part }) => ({
repoPath: part === 'base' ? info.baseRepoPath || info.repoPath : info.repoPath,
commitID: part === 'base' ? info.baseCommitID! : info.commitID,
filePath: part === 'base' ? info.baseFilePath! : info.filePath,
rev: part === 'base' ? info.baseRev || info.baseCommitID! : info.rev || info.commitID,
})

hoverifier.hoverify({
dom,
positionEvents: of(codeView).pipe(findPositionsFromEvents(dom)),
resolveContext,
adjustPosition,
})
subscriptions.add(
hoverifier.hoverify({
dom,
positionEvents: of(codeView).pipe(findPositionsFromEvents(dom)),
resolveContext,
adjustPosition,
})
)

codeView.classList.add('sg-mounted')

if (!getToolbarMount) {
return
Expand All @@ -253,4 +283,32 @@ export function injectCodeIntelligence(codeHostInfo: CodeHost): Subscription {
)
})
)

return subscriptions
}

async function injectCodeIntelligenceToCodeHosts(codeHosts: CodeHost[]): Promise<Subscription> {

Choose a reason for hiding this comment

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

An async function that returns Subscriptions on first sight seems like a weird mix of patterns to me

Copy link
Author

Choose a reason for hiding this comment

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

Should I not make this an async function?

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Yea, I'll add some docs

const subscriptions = new Subscription()

for (const codeHost of codeHosts) {
const isCodeHost = await Promise.resolve(codeHost.check())
if (isCodeHost) {
subscriptions.add(handleCodeHost(codeHost))
}
}

return subscriptions
}

/**
* Injects all code hosts into the page.
*
* @returns A promise with a subscription containing all subscriptions for code
* intelligence. Unsubscribing will clean up subscriptions for hoverify and any
* incomplete setup requests.
*/
export async function injectCodeIntelligence(): Promise<Subscription> {
const codeHosts: CodeHost[] = [githubCodeHost, phabricatorCodeHost]

return await injectCodeIntelligenceToCodeHosts(codeHosts)
}
123 changes: 123 additions & 0 deletions src/libs/code_intelligence/code_views.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
import { from, merge, Observable, of, Subject } from 'rxjs'
import { filter, map, mergeMap } from 'rxjs/operators'

import { CodeHost, ResolvedCodeView } from './code_intelligence'

/**
* Emits a ResolvedCodeView when it's DOM element is on or about to be on the page.
*/
const emitWhenIntersecting = (margin: number) => {
const codeViewStash = new Map<HTMLElement, ResolvedCodeView>()

const intersectingElements = new Subject<HTMLElement>()

const intersectionObserver = new IntersectionObserver(
entries => {
for (const entry of entries) {
// `entry` is an `IntersectionObserverEntry`,
// which has
// [isIntersecting](https://developer.mozilla.org/en-US/docs/Web/API/IntersectionObserverEntry/isIntersecting#Browser_compatibility)
// as a prop, but TS complains that it does not
// exist.
if ((entry as any).isIntersecting) {
intersectingElements.next(entry.target as HTMLElement)

Choose a reason for hiding this comment

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

Instead of Subjects, prefer the Observable constructor

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

I tried using the observable constructor but ended up here

}
}
},
{
rootMargin: `${margin}px`,
threshold: 0,
}
)

return (codeViews: Observable<ResolvedCodeView>) =>
new Observable<ResolvedCodeView>(observer => {
codeViews.subscribe(({ codeView, ...rest }) => {
intersectionObserver.observe(codeView)

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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?

codeViewStash.set(codeView, { codeView, ...rest })
})

intersectingElements
.pipe(
map(element => codeViewStash.get(element)),
filter(codeView => !!codeView)
)
.subscribe(observer)
})
}

/**
* findCodeViews finds all the code views on a page given a CodeHost. It emits code views
* that are lazily loaded as well.
*/
export const findCodeViews = (codeHost: CodeHost, watchChildrenModifications = true) => (
containers: Observable<Element>
) => {
const codeViewsFromList: Observable<ResolvedCodeView> = containers.pipe(
filter(() => !!codeHost.codeViews),
mergeMap(container =>
from(codeHost.codeViews!).pipe(
map(({ selector, ...info }) => ({
info,
matches: container.querySelectorAll<HTMLElement>(selector),
}))
)
),
mergeMap(({ info, matches }) =>
of(...matches).pipe(

Choose a reason for hiding this comment

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

from creates Observables from arrays

map(codeView => ({
...info,
codeView,
}))
)
)
)

const codeViewsFromResolver: Observable<ResolvedCodeView> = containers.pipe(
filter(() => !!codeHost.codeViewResolver),
map(container => ({
resolveCodeView: codeHost.codeViewResolver!.resolveCodeView,
matches: container.querySelectorAll<HTMLElement>(codeHost.codeViewResolver!.selector),
})),
mergeMap(({ resolveCodeView, matches }) =>
of(...matches).pipe(
map(codeView => ({
...resolveCodeView(codeView),
codeView,
}))
)
)
)

const obs = [codeViewsFromList, codeViewsFromResolver]

if (watchChildrenModifications) {
const possibleLazilyLoadedContainers = new Subject<HTMLElement>()

const mutationObserver = new MutationObserver(mutations => {
for (const mutation of mutations) {
if (mutation.type === 'childList' && mutation.target instanceof HTMLElement) {
const { target } = mutation

possibleLazilyLoadedContainers.next(target)
}
}
})

containers.subscribe(container =>
mutationObserver.observe(container, {
childList: true,
subtree: true,
})
)

const lazilyLoadedCodeViews = possibleLazilyLoadedContainers.pipe(findCodeViews(codeHost, false))

obs.push(lazilyLoadedCodeViews)
}

return merge(...obs).pipe(
emitWhenIntersecting(250),
filter(({ codeView }) => !codeView.classList.contains('sg-mounted'))
)
}
1 change: 1 addition & 0 deletions src/libs/code_intelligence/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from './code_intelligence'
Loading