From 5a27517c7c654582e49d183d47959458ad8bed89 Mon Sep 17 00:00:00 2001 From: Stephen Gutekanst Date: Tue, 23 Oct 2018 23:21:37 -0700 Subject: [PATCH] fix: command palette A regression (#262) which broke the command palette was introduced (but not published). This fixes that regression. - The regression was introduced by @sqs's change to e-c-c here: https://github.com/sourcegraph/extensions-client-common/commit/19df39f8a349c38242f96d0aeef4c78b70cb20b3 - The dependency was updated in this repository by @chrismwendt here: https://github.com/sourcegraph/browser-extensions/commit/8616d6641b7bbf6407e632a0c21a431d93f760c0 The issue is that we are using a `@shopify/react-shortcuts` package to declare keyboard shortcuts in e-c-c. The package registers keyboard shortcuts via a `` component which requires that a `` component be somewhere above it in the React rendering tree, in order to pass down some React context. In the Sourcegraph web application, we provide such an implementation: https://sourcegraph.sgdev.org/github.com/sourcegraph/sourcegraph/-/blob/src/SourcegraphWebApp.tsx#L259:14 Here in the browser extension, however, we do not -- and this causes any `` component to break due to missing React context data. If we were to publish the browser extension today, the Sourcegraph Extension command palette would be broken outright (clicking on it causes it to disappear). Fixing the issue is tricky: `@shopify/react-shortcuts`'s `ShortcutProvider` component is designed with the assumption that there will only ever be a single React component rendering the entire application. It uses this assumption to only register a single document keyboard event handler, [otherwise it will install multiple global event handlers to the `document` that conflict with one another](https://github.com/Shopify/quilt/blob/master/packages/react-shortcuts/src/ShortcutManager/ShortcutManager.tsx#L21). In most applications, this assumption holds true, but in our situation here with the browser extension this is not true as we inject independent React views into several different locations on the page and multiple expect to be able to add ``s. Fixing this becomes more tricky because `@shopify/react-shortcuts` does not expose [the `Provider` part of the `React.createContext` result](https://github.com/Shopify/quilt/blob/master/packages/react-shortcuts/src/ShortcutProvider/ShortcutProvider.tsx#L12) -- meaning it is impossible for us to implement our own `ShortcutProvider` that fixes this issue. I've had to temporarily fork react-shortcuts to expose this, and if all looks good I will create a PR upstream for this: https://github.com/slimsag/quilt/tree/publish-provider In the meantime, we will have to use my fork everywhere (if we don't, React context will not grab the right data and it'll e.g. break the command palette on Sourcegraph.com). This does not actually cause us to register shortcuts in the browser extension for the command palette -- but we certainly can now after this change. See also: - https://github.com/sourcegraph/extensions-client-common/pull/60 - https://github.com/sourcegraph/sourcegraph/pull/500 Fixes #262 --- package.json | 1 + src/libs/code_intelligence/extensions.tsx | 13 ++++--- src/libs/github/extensions.tsx | 13 ++++--- src/shared/components/GlobalDebug.tsx | 19 ++++++---- src/shared/components/ShortcutProvider.tsx | 42 ++++++++++++++++++++++ yarn.lock | 7 ++++ 6 files changed, 78 insertions(+), 17 deletions(-) create mode 100644 src/shared/components/ShortcutProvider.tsx diff --git a/package.json b/package.json index 14e19e69..9da554e9 100644 --- a/package.json +++ b/package.json @@ -105,6 +105,7 @@ "worker-loader": "^2.0.0" }, "dependencies": { + "@slimsag/react-shortcuts": "^1.2.1", "@sourcegraph/codeintellify": "^3.9.0", "@sourcegraph/extensions-client-common": "^10.0.0", "@sourcegraph/react-loading-spinner": "0.0.6", diff --git a/src/libs/code_intelligence/extensions.tsx b/src/libs/code_intelligence/extensions.tsx index dd63d9f8..b0f03a3e 100644 --- a/src/libs/code_intelligence/extensions.tsx +++ b/src/libs/code_intelligence/extensions.tsx @@ -25,6 +25,7 @@ import * as H from 'history' import { isErrorLike } from '../../shared/backend/errors' import { createExtensionsContextController, createMessageTransports } from '../../shared/backend/extensions' import { GlobalDebug } from '../../shared/components/GlobalDebug' +import { ShortcutProvider } from '../../shared/components/ShortcutProvider' import { sourcegraphUrl } from '../../shared/util/context' import { getGlobalDebugMount } from '../github/extensions' import { MountGetter } from './code_intelligence' @@ -111,11 +112,13 @@ export function initializeExtensions( const { extensionsContextController, extensionsController } = createControllers(documents) render( - , + + + , getCommandPaletteMount() ) diff --git a/src/libs/github/extensions.tsx b/src/libs/github/extensions.tsx index bc5f765b..54dff167 100644 --- a/src/libs/github/extensions.tsx +++ b/src/libs/github/extensions.tsx @@ -8,6 +8,7 @@ import { ContributableMenu } from 'sourcegraph/module/protocol' import * as React from 'react' import { render } from 'react-dom' import { GlobalDebug } from '../../shared/components/GlobalDebug' +import { ShortcutProvider } from '../../shared/components/ShortcutProvider' export function getCommandPaletteMount(): HTMLElement { const headerElem = document.querySelector('div.HeaderMenu>div:last-child') @@ -51,11 +52,13 @@ export function injectExtensionsGlobalComponents({ extensionsContextController: Controller }): void { render( - , + + + , getCommandPaletteMount() ) diff --git a/src/shared/components/GlobalDebug.tsx b/src/shared/components/GlobalDebug.tsx index 0d8f3a87..090606d5 100644 --- a/src/shared/components/GlobalDebug.tsx +++ b/src/shared/components/GlobalDebug.tsx @@ -6,6 +6,7 @@ import * as H from 'history' import MenuDownIcon from 'mdi-react/MenuDownIcon' import * as React from 'react' import { sourcegraphUrl } from '../util/context' +import { ShortcutProvider } from './ShortcutProvider' interface Props { location: H.Location @@ -28,13 +29,17 @@ export const GlobalDebug: React.SFC = props =>
  • - void }>} - caretIcon={MenuDownIcon as React.ComponentType<{ className: string; onClick?: () => void }>} - extensionsController={props.extensionsController} - link={ExtensionLink} - /> + + void }> + } + caretIcon={MenuDownIcon as React.ComponentType<{ className: string; onClick?: () => void }>} + extensionsController={props.extensionsController} + link={ExtensionLink} + /> +
diff --git a/src/shared/components/ShortcutProvider.tsx b/src/shared/components/ShortcutProvider.tsx new file mode 100644 index 00000000..2b2e41b7 --- /dev/null +++ b/src/shared/components/ShortcutProvider.tsx @@ -0,0 +1,42 @@ +import { Context, ContextProvider, ProviderProps, ShortcutManager } from '@slimsag/react-shortcuts' +import * as React from 'react' + +/** + * Describes the variable this file injects into the `global` object. It is + * heavily prefixed to avoid collisions. + */ +interface GlobalContext { + /** The singleton ShortcutManager object. */ + browserExtensionShortcutManager?: ShortcutManager +} + +// This ShortcutProvider is derived from the default @shopify/react-shortcuts +// implementation: +// +// https://github.com/Shopify/quilt/blob/master/packages/react-shortcuts/src/ShortcutProvider/ShortcutProvider.tsx +// +// We cannot use the default implementation above because it assumes the +// application is rendered via a single React component in order to create the +// ShortcutManager singleton. In our case, there are multiple React components +// on the page and they each need to use a single ShortcutManager, so we must +// manage it ourselves here. If we did not do this, we would have multiple +// ShortcutManagers and each would register their own conflicting document +// event handlers. +export class ShortcutProvider extends React.Component { + public componentDidMount(): void { + const globals = global as GlobalContext + if (!globals.browserExtensionShortcutManager) { + globals.browserExtensionShortcutManager = new ShortcutManager() + globals.browserExtensionShortcutManager.setup() + } + } + + public render(): JSX.Element | null { + const globals = global as GlobalContext + const context: Context = { + shortcutManager: globals.browserExtensionShortcutManager, + } + + return {this.props.children} + } +} diff --git a/yarn.lock b/yarn.lock index f6e16363..13382de7 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1306,6 +1306,13 @@ dependencies: prop-types "^15.6.2" +"@slimsag/react-shortcuts@^1.2.1": + version "1.2.1" + resolved "https://registry.yarnpkg.com/@slimsag/react-shortcuts/-/react-shortcuts-1.2.1.tgz#a3ea054a057137de8636bc9fabac61369103e597" + integrity sha512-dUTQjBX1yjnHbqkTL5ditqtx5R6QhcXI3lCnjOy8e5XUOr5LOtVpol1mufU2lAb7DtEYrSVRQpUeNmZrTEgLcg== + dependencies: + prop-types "^15.6.2" + "@sourcegraph/codeintellify@^3.9.0": version "3.9.0" resolved "https://registry.yarnpkg.com/@sourcegraph/codeintellify/-/codeintellify-3.9.0.tgz#1e3f002558cc367e8c5ee9b0a54e0c47c6ee8761"