feat: accept settings updates from Sourcegraph pages#146
Conversation
chrismwendt
force-pushed
the
settings-from-sourcegraph
branch
2 times, most recently
from
be4f84e to
b001513
Compare
src/libs/sourcegraph/inject.tsx
Outdated
| * Where a message originated from, used to disambiguate between the content | ||
| * script's own messages and messages from the page. | ||
| */ | ||
| type Source = 'Page' | 'Client' |
There was a problem hiding this comment.
Do these need to be synced with what is in your ecc PR? Can't they share?
There was a problem hiding this comment.
Yes, they can share. I haven't updated this PR, but I will once the e-c-c PR gets merged in.
src/libs/sourcegraph/inject.tsx
Outdated
| * pings back. This always resolves to true. | ||
| */ | ||
| function connectToPage(): Promise<boolean> { | ||
| return new Promise(resolve => { |
There was a problem hiding this comment.
Minimize the logic in a Promise constructor:
new Promise(resolve => window.addEventListenter('message', resolve)
.then(...)
src/libs/sourcegraph/inject.tsx
Outdated
| } | ||
|
|
||
| let isConnected = false | ||
| window.addEventListener('message', event => { |
There was a problem hiding this comment.
This doesn't cleanup the listener after the Promise is resolved, so nothing here can be garbage collected. I would recommend just using fromEvent+filter+toPromise.
chrismwendt
force-pushed
the
settings-from-sourcegraph
branch
3 times, most recently
from
eb8fd9e to
27ae8c0
Compare
| } | ||
|
|
||
| // Generate and insert DOM element, in case this code executes first. | ||
| document.body.appendChild(marker) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
This function only gets called when the DOM is loaded:
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah, I see. Not exactly sure why it uses the load event but might have a reason.
chrismwendt
force-pushed
the
settings-from-sourcegraph
branch
from
27ae8c0 to
bf8b125
Compare
| } | ||
| return new Promise<undefined>(resolve => | ||
| storage.setSync({ clientSettings }, () => { | ||
| resolve(undefined) |
There was a problem hiding this comment.
Can't you pass resolve directly? Since the callback has no parameters, it should fit the type void
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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")
There was a problem hiding this comment.
Like this? new Promise(resolve => setSync(..., resolve)).then(() => undefined)
There was a problem hiding this comment.
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)
|
|
||
| if (document.readyState === 'complete' || document.readyState === 'interactive') { | ||
| dispatchSourcegraphEvents(marker) | ||
| dispatchSourcegraphEvents() |
There was a problem hiding this comment.
Doesn't this fire the event twice portentially?
|
🎉 This PR is included in version 1.14.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
This connects the browser extension to Sourcegraph pages so that Sourcegraph can instruct the browser extension to edit client settings on its behalf.
This will allow us to eliminate the extensions list from the browser extension (see #132) and from e-c-c (see TODO).
I didn't piggyback on the existing
document.dispatchEventcode because we might want to embed Sourcegraph in an iframe, in which case I think the origin will be different.