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
34 changes: 10 additions & 24 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@
},
"dependencies": {
"@sourcegraph/codeintellify": "^3.8.3",
"@sourcegraph/extensions-client-common": "^8.1.4",
"@sourcegraph/extensions-client-common": "^8.2.0",
"@sourcegraph/react-loading-spinner": "0.0.6",
"@sqs/jsonc-parser": "^1.0.3",
"bootstrap": "^4.0.0",
Expand Down
34 changes: 29 additions & 5 deletions src/libs/sourcegraph/inject.tsx
Original file line number Diff line number Diff line change
@@ -1,20 +1,44 @@
import { connectAsClient } from '@sourcegraph/extensions-client-common/lib/messaging'
import storage from '../../browser/storage'
import { updateExtensionSettings } from '../../shared/backend/extensions'

export function injectSourcegraphApp(marker: HTMLElement): void {
if (document.getElementById(marker.id)) {
return
}

// Generate and insert DOM element, in case this code executes first.
document.body.appendChild(marker)
Copy link
Author

Choose a reason for hiding this comment

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

This marker used to only be added at DOM load, which created a window of time during which this inject function was being called twice.

Choose a reason for hiding this comment

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

If it doesn't wait for DOM load, how do you ensure that document.body has already loaded? Afaik it's not safe to do DOM manipulation until the DOM is loaded

Copy link
Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Then I don't understand your above comment. Was the issue that this event handler would get dispatched before the one that adds the marker, and now both add the marker?

Copy link
Author

Choose a reason for hiding this comment

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

Oh yeah, what I said was ambiguous. If you look at the red part of the diff where this statement used to be, you'll see that it's inside a callback to the load event, which executes asynchronously with respect to this function call.

Choose a reason for hiding this comment

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

Ah, I see. Not exactly sure why it uses the load event but might have a reason.


connectAsClient()
.then(connection => {
storage.observeSync('clientSettings').subscribe(settings => {
connection.sendSettings(settings)
})

connection.onEditSetting(edit => updateExtensionSettings('Client', edit).toPromise())

connection.onGetSettings(
() =>
new Promise<string>(resolve => {
storage.getSync(storageItems => {
resolve(storageItems.clientSettings)
})
})
)
})
.catch(error => console.error(error))

window.addEventListener('load', () => {
dispatchSourcegraphEvents(marker)
dispatchSourcegraphEvents()
})

if (document.readyState === 'complete' || document.readyState === 'interactive') {
dispatchSourcegraphEvents(marker)
dispatchSourcegraphEvents()

Choose a reason for hiding this comment

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

Doesn't this fire the event twice portentially?

Copy link
Author

Choose a reason for hiding this comment

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

Looks like it to me, yeah

}
}

function dispatchSourcegraphEvents(marker: HTMLElement): void {
// Generate and insert DOM element, in case this code executes first.
document.body.appendChild(marker)
function dispatchSourcegraphEvents(): void {
// Send custom webapp <-> extension registration event in case webapp listener is attached first.
document.dispatchEvent(new CustomEvent<{}>('sourcegraph:browser-extension-registration'))
}
84 changes: 42 additions & 42 deletions src/shared/backend/extensions.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { UpdateExtensionSettingsArgs } from '@sourcegraph/extensions-client-common/lib/context'
import { Controller as ExtensionsContextController } from '@sourcegraph/extensions-client-common/lib/controller'
import { ConfiguredExtension } from '@sourcegraph/extensions-client-common/lib/extensions/extension'
import { gql, graphQLContent } from '@sourcegraph/extensions-client-common/lib/graphql'
Expand All @@ -17,15 +18,14 @@ import { isEqual } from 'lodash'
import Alert from 'mdi-react/AlertIcon'
import MenuDown from 'mdi-react/MenuDownIcon'
import Menu from 'mdi-react/MenuIcon'
import { combineLatest, Observable, Subject, throwError } from 'rxjs'
import { combineLatest, from, Observable, throwError } from 'rxjs'
import { distinctUntilChanged, map, mergeMap, switchMap, take } from 'rxjs/operators'
import { ClientOptions } from 'sourcegraph/module/client/client'
import { MessageTransports } from 'sourcegraph/module/jsonrpc2/connection'
import { TextDocumentDecoration } from 'sourcegraph/module/protocol'
import { ConfigurationUpdateParams } from 'sourcegraph/module/protocol'
import uuid from 'uuid'
import { Disposable } from 'vscode-languageserver'
import storage from '../../browser/storage'
import storage, { StorageItems } from '../../browser/storage'
import { ExtensionConnectionInfo } from '../../extension/scripts/background'
import { onFirstMessage } from '../../extension/scripts/background'
import { getContext } from './context'
Expand Down Expand Up @@ -255,45 +255,7 @@ export function createExtensionsContextController(
map(([gqlCascade, storageCascade]) => mergeCascades(gqlToCascade(gqlCascade), storageCascade)),
distinctUntilChanged((a, b) => isEqual(a, b))
),
updateExtensionSettings: (
subjectID,
args: { extensionID: string; edit?: ConfigurationUpdateParams; enabled?: boolean; remove?: boolean }
) => {
if (subjectID !== 'Client') {
return throwError('Cannot update settings for ' + subjectID + '.')
}
const update = new Subject<undefined>()
storage.getSync(storageItems => {
let clientSettings = storageItems.clientSettings

const format = { tabSize: 2, insertSpaces: true, eol: '\n' }

if (args.edit) {
clientSettings = applyEdits(
clientSettings,
// TODO(chris): remove `.slice()` (which guards against
// mutation) once
// https://github.com/Microsoft/node-jsonc-parser/pull/12
// is merged in.
setProperty(clientSettings, args.edit.path.slice(), args.edit.value, format)
)
} else if (typeof args.enabled === 'boolean') {
clientSettings = applyEdits(
clientSettings,
setProperty(clientSettings, ['extensions', args.extensionID], args.enabled, format)
)
} else if (args.remove) {
clientSettings = applyEdits(
clientSettings,
removeProperty(clientSettings, ['extensions', args.extensionID], format)
)
}
storage.setSync({ clientSettings }, () => {
update.next(undefined)
})
})
return update
},
updateExtensionSettings,
queryGraphQL: (request, variables) =>
storage.observeSync('sourcegraphURL').pipe(
take(1),
Expand All @@ -315,3 +277,41 @@ export function createExtensionsContextController(
},
})
}

export const updateExtensionSettings = (subjectID, args: UpdateExtensionSettingsArgs): Observable<undefined> => {
if (subjectID !== 'Client') {
return throwError('Cannot update settings for ' + subjectID + '.')
}
return from(
new Promise<StorageItems>(resolve => storage.getSync(storageItems => resolve(storageItems))).then(
storageItems => {
let clientSettings = storageItems.clientSettings

const format = { tabSize: 2, insertSpaces: true, eol: '\n' }

if ('edit' in args && args.edit) {
clientSettings = applyEdits(
clientSettings,
// TODO(chris): remove `.slice()` (which guards against
// mutation) once
// https://github.com/Microsoft/node-jsonc-parser/pull/12
// is merged in.
setProperty(clientSettings, args.edit.path.slice(), args.edit.value, format)
)
} else if ('extensionID' in args) {
clientSettings = applyEdits(
clientSettings,
typeof args.enabled === 'boolean'
? setProperty(clientSettings, ['extensions', args.extensionID], args.enabled, format)
: removeProperty(clientSettings, ['extensions', args.extensionID], format)
)
}
return new Promise<undefined>(resolve =>
storage.setSync({ clientSettings }, () => {
resolve(undefined)

Choose a reason for hiding this comment

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

Can't you pass resolve directly? Since the callback has no parameters, it should fit the type void

Copy link
Author

Choose a reason for hiding this comment

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

I generally avoid that because it makes me less confident that the return value of the Promise will be what I intend for it to be.

Choose a reason for hiding this comment

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

Got it, if that's a concern I would eliminate it with in a different way with .then(() => undefined). That keeps logic in Promise constructors minimal (no point for the next dev to think "I need to add this logic, I can just add it here before the resolve call")

Copy link
Author

Choose a reason for hiding this comment

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

Like this? new Promise(resolve => setSync(..., resolve)).then(() => undefined)

Choose a reason for hiding this comment

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

Yeah. (Except if it's inside another .then() handler, in which case the .then() should be on the outer Promise to not nest them, but same thing)

})
)
}
)
)
}
16 changes: 11 additions & 5 deletions src/shared/components/options/ConnectionCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,17 @@ export class ConnectionCard extends React.Component<Props, State> {
}
}

private setContentScriptUrls(props: Props): void {
this.contentScriptUrls = [...props.storage.clientConfiguration.contentScriptUrls, props.storage.sourcegraphURL]
}

public componentDidMount(): void {
this.contentScriptUrls = this.props.storage.clientConfiguration.contentScriptUrls
this.setContentScriptUrls(this.props)
this.checkConnection()
}

public componentWillReceiveProps(nextProps: Props): void {
this.contentScriptUrls = nextProps.storage.clientConfiguration.contentScriptUrls
this.setContentScriptUrls(nextProps)
}

private sourcegraphServerAlert = (): JSX.Element => {
Expand All @@ -74,12 +78,14 @@ export class ConnectionCard extends React.Component<Props, State> {
</div>
)
}
const hasPermissions = this.contentScriptUrls.every(val => permissionOrigins.indexOf(`${val}/*`) >= 0)
if (!hasPermissions && !permissionOrigins.includes('<all_urls>')) {
const forbiddenUrls = permissionOrigins.includes('<all_urls>')
? []
: this.contentScriptUrls.filter(url => !permissionOrigins.includes(`${url}/*`))
if (forbiddenUrls.length !== 0) {
return (
<div className="pt-2">
<Alert color="warning">
{`Missing content script permissions: ${this.contentScriptUrls.join(', ')}.`}
{`Missing content script permissions: ${forbiddenUrls.join(', ')}.`}
<div className="pt-2">
<Button
onClick={this.requestPermissions}
Expand Down