This repository has been archived by the owner on Jan 22, 2019. It is now read-only.
latest master: Extensions command palette broken in the browser extension after e-c-c update #262
Assignees
Labels
Comments
slimsag
added
bug
slimsag
added a commit
that referenced
this issue
Oct 24, 2018
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: sourcegraph/extensions-client-common@19df39f - The dependency was updated in this repository by @chrismwendt here: 8616d66 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 `<Shortcut>` component which requires that a `<ShortcutProvider>` 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 `<Shortcut>` 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 `<Shortcut>`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: - sourcegraph/extensions-client-common#60 - sourcegraph/sourcegraph#500 Fixes #262
slimsag
added a commit
that referenced
this issue
Oct 24, 2018
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: sourcegraph/extensions-client-common@19df39f - The dependency was updated in this repository by @chrismwendt here: 8616d66 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 `<Shortcut>` component which requires that a `<ShortcutProvider>` 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 `<Shortcut>` 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 `<Shortcut>`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: - sourcegraph/extensions-client-common#60 - sourcegraph/sourcegraph#500 Fixes #262
slimsag
added a commit
that referenced
this issue
Oct 24, 2018
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: sourcegraph/extensions-client-common@19df39f - The dependency was updated in this repository by @chrismwendt here: 8616d66 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 `<Shortcut>` component which requires that a `<ShortcutProvider>` 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 `<Shortcut>` 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 `<Shortcut>`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: - sourcegraph/extensions-client-common#60 - sourcegraph/sourcegraph#500 Fixes #262
|
🎉 This issue has been resolved in version 1.18.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Running the latest master of the
browser-extensionsrepository shows that clicking the extensions command palette button no longer works. On click, it just disappears:Console errors appear:
I am working on this.
The text was updated successfully, but these errors were encountered: