external services UI #1103
external services UI #1103
Conversation
nicksnyder
force-pushed
the
extsvcui
branch
2 times, most recently
from
a0d1861
to
7bd38d2
Compare
|
I would like to merge this today but I assume there will be review comments. Please be clear about which comments you think block merging. In the very short term this UI will use useful in dogfood and sourcegraph.com to help me migrate. |
|
What is an "external service"? Is it a code host? Or what else can it "service"? |
|
Yes, they are basically code hosts for now. See #1077 (comment) |
|
+1 Nice |
|
Sorry I haven't been involved in this project at all, but I'm a bit confused. Is this replacing having code host connections in site config? Or will there just be two UIs for adding them now? |
|
@dadlerj it is the replacement long term, but I think temporarily we will have both (not sure what we will have for 3.0 preview though) |
Yes, we are moving this out of site config. Users will never be in a state where there are two sources of truth. |
|
@felixfbecker can you review my resolutions to your comments? |
| }, | ||
| ].concat(extraSchemas), | ||
| }) | ||
| setDiagnosticsOptions(monaco, this.props) |
There was a problem hiding this comment.
Above you use this.monaco, here you use monaco. I think you need to use this.monaco everywhere in all value positions or else monaco will not be lazy-loaded but bundled into the main bundle. The import monaco is only for the type I think. To prevent these mistakes I usually do something like
import * as _monaco from 'monaco'
type Monaco = typeof _monaco // Type only!With the underscore making it clear that this should not be used anywhere.
There was a problem hiding this comment.
There's also now TS import types which can import a type without creating a value binding:
interface State {
monaco: import('monaco'),
}There was a problem hiding this comment.
monaco is just a pre-existing variable that is assigned on L144, so I don't think I have changed any semantics with respect to how monaco is imported here.
Codecov Report
|
Part of #914
