Skip to content
This repository was archived by the owner on Jan 22, 2019. It is now read-only.

feat: support Sourcegraph extensions in the new inject code#199

Merged
ijsnow merged 15 commits intomasterfrom
ext-new-inject
Sep 28, 2018
Merged

feat: support Sourcegraph extensions in the new inject code#199
ijsnow merged 15 commits intomasterfrom
ext-new-inject

Conversation

@chrismwendt
Copy link

  • Adds support for Sourcegraph extensions with the newInject feature flag
  • Adds support for hovers/defs for Sourcegraph extensions on diff pages
  • Bumps e-c-c and sourcegraph-extension-api

@chrismwendt chrismwendt requested review from ijsnow and sqs September 27, 2018 18:50
extensionsController: ClientController<ConfigurationSubject, Settings>
extensionsContextController: Controller<ConfigurationSubject, Settings>
}): void {
const headerElem = document.querySelector('div.HeaderMenu>div:last-child')
Copy link

@ijsnow ijsnow Sep 27, 2018

Choose a reason for hiding this comment

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

@chrismwendt This isn't abstract. Nothing inside libs/code_intelligence should know anything code host specific. That's why I had created a getCommandPaletteMount function that the code host has to implement.

Copy link
Author

Choose a reason for hiding this comment

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

Oh right, that was unintentional and definitely should be factored out.


interface Controllers {
ctx: Controller<ConfigurationSubject, Settings>
ctl: ClientController<ConfigurationSubject, Settings>
Copy link

Choose a reason for hiding this comment

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

Could we have a little more specific names/doc blocks? Since the interface is named Controllers, ctx and ctl just confuses me especially considering the types look like they indeed are both controllers

@chrismwendt
Copy link
Author

This PR breaks the extensions list in the options page (due to sourcegraph/extensions-client-common@7f1d38c which got into e-c-c v8.6.0, and this PR bumps to v8.8.0).

I'm working on fixing this now.

@ijsnow ijsnow changed the title feat: support Sourcegraph extensions in the new inject code WIP feat: support Sourcegraph extensions in the new inject code Sep 28, 2018
@ijsnow
Copy link

ijsnow commented Sep 28, 2018

TODO: Implement CodeView.getLineRanges for GitHub and Phabricator, or make it optional behind an extensions field that is optional.

@chrismwendt
Copy link
Author

Fixed the extensions list page and pushed.

@ijsnow ijsnow changed the title WIP feat: support Sourcegraph extensions in the new inject code feat: support Sourcegraph extensions in the new inject code Sep 28, 2018
@ijsnow ijsnow merged commit d2ce8e4 into master Sep 28, 2018
@ijsnow ijsnow deleted the ext-new-inject branch September 28, 2018 22:37
@sourcegraph-bot
Copy link

🎉 This PR is included in version 1.16.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.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants