-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: proxy & browser capabilities #2
Conversation
|
||
const config: ForgeConfig = { | ||
packagerConfig: { | ||
asar: true, | ||
extraResource: [ | ||
'./resources/json_output.py', | ||
'./resources/' + getPlatform() + '/' + getArch(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we will pick up only the specific os/architecture combination when packaging for a specific system
@@ -0,0 +1,3 @@ | |||
declare interface Window { | |||
studio: import('../src/preload').Studio; | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the trick for not having to specify manually every type
id: string, | ||
request: Request, | ||
response?: Response, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this has more data but like this is enough to get us started
content_hash = None | ||
|
||
# we base64 encode content and let the client deal with it depending on mimetype | ||
content = base64.b64encode(flow.response.content).decode() if flow.response.content else None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using base64 encoding for "stringifying" binary data since it doesn't play well with the json format. This should allow us to handle all mime-types and eventually showcase them in a better way 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments, mostly questions/nitpicks, but with 2 highlighting potential bugs in types
src/browser.ts
Outdated
const createUserDataDir = async () => { | ||
return await mkdtemp(path.join(os.tmpdir(), 'k6-studio-')); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: async
and await
are redundant here as long as mkdtemp
returns a promise.
src/browser.ts
Outdated
const userDataDir = await createUserDataDir(); | ||
console.log(userDataDir); | ||
const certificateSPKI = await getCertificateSPKI(); | ||
const disableChromeOptimizations = "--disable-features=OptimizationGuideModelDownloading,OptimizationHintsFetching,OptimizationTargetPrediction,OptimizationHints"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit/future improvement: IMO having an array of disabled features, that can then be used to create this string will make it a bit more readable and easier to maintain:
// Could probably be an export from src/constants.ts or some other file with constants, but it's not super important
const DISABLED_CHROME_FEATURES = [
"OptimizationGuideModelDownloading",
"OptimizationHintsFetching",
"OptimizationTargetPrediction",
"OptimizationHints",
]
// ...
const disableChromeOptimizations = `--disable-features=${DISABLED_CHROME_FEATURES.join(',')}`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add a TODO, I also would like to have every flag passed to the chromium browsers to be documented so that we know the "why" they are there
src/lib/types.ts
Outdated
status_code: number, | ||
content: string, | ||
path: string, | ||
timestamp_start: Date, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Date
is not serializable. It should probably be string
src/lib/types.ts
Outdated
method: string, | ||
path: string, | ||
content: string, | ||
timestamp_start: Date, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Date
is not serializable. It should probably be string
|
||
const proxy = { | ||
launchProxy: () => { | ||
ipcRenderer.send('proxy:start'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is an actually confusing bit about electron. As of today per my understanding you have both the .send()/.on()
and .invoke()/.handle()
methods combination.
For single direction communication is totally fine to use .send()
and it's actually more explicit because you are saying that you are not expecting a response.
For bi-directional communication, although you can still do the old pattern with .send()
it is recommended to use the .invoke()
method when you expect a response back.
So .send()
it's not a legacy approach on every scenario but just in the bi-directional communication part (where you find the warning).
src/lib/types.ts
Outdated
export interface Response { | ||
headers: Array<Array<string>>; | ||
reason: string, | ||
status_code: number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: At this point, we're used to snake_case on the frontend, but camelCase is way more common in the JS/TS world, so maybe we can consider it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes! This represents data from the proxy exposed in this way, we can try to change the casing either now or in Phase 2 🤔
|
||
const proxy = { | ||
launchProxy: () => { | ||
ipcRenderer.send('proxy:start'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is an actually confusing bit about electron. As of today per my understanding you have both the .send()/.on()
and .invoke()/.handle()
methods combination.
For single direction communication is totally fine to use .send()
and it's actually more explicit because you are saying that you are not expecting a response.
For bi-directional communication, although you can still do the old pattern with .send()
it is recommended to use the .invoke()
method when you expect a response back.
So .send()
it's not a legacy approach on every scenario but just in the bi-directional communication part (where you find the warning).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
Events
Introduce the logic for handling both the proxy and the browser via events.
Events are prefixed by the resource they work on.
Proxy Events:
proxy:start
proxy:started
proxy:stop
proxy:data
: data received by the proxy, representing requests/responsesBrowser Events:
browser:start
browser:started
browser:stop
The events are separated to maintain fine control and allow for future optimizations (for example, we can start the proxy before the user actually uses it, improving startup time)
Binaries
The binaries for the proxy are under the path
resources
, they will be retrieved later on depending on the os/architecture combination viaprocess.resourcePath
.Certificates
The certificate for the mitm proxying is created by the proxy on the user machine when run for the first time. We retrieve this file and generate the SPKI to pass to the browser for accepting it. This allows us to avoid shipping a pre-created certificate that could be misused.
Example
index.html
&renderer.ts
showcase how to use those events for dealing with both the proxy & browser, it should be fairly straightforwrd.json_output.py
is the logic converting the proxy captured data into json for passing it to the node process, it will still need more work for handling different mime-types.Types
For the types of the bridged context in
window
I've opted for the automatic approach, this should speedup development initially, but we can consider swapping back to manual typing 🤔