Skip to content
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: Prep for v1 #19

Merged
merged 77 commits into from
Jan 2, 2021
Merged

feat: Prep for v1 #19

merged 77 commits into from
Jan 2, 2021

Conversation

ebebbington
Copy link
Member

@ebebbington ebebbington commented Oct 18, 2020

Fixes #7

Description

  • Uses websockets instead of sub process to communicate

Work

  • Rewrite codebase to support web sockets instead
  • Tests
  • Make api scalable so it would be possible to use auth
  • Update docs
  • add website docs
  • any others?

@ebebbington ebebbington changed the title [WIP] feat: Prep for v2 [WIP] feat: Prep for v1 Oct 18, 2020
interface MessageResponse { // For when we send an event to get one back, eg running a JS expression
id: number;
result?: {
result: { [key: string]: unknown };

Choose a reason for hiding this comment

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

FYI; not all responses have an "inner" result, its common but not always.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok cheers, gotcha 👍 thanks !

Copy link
Member Author

Choose a reason for hiding this comment

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

@ebebbington
Copy link
Member Author

OMG THE TESTS PASS

@ebebbington ebebbington marked this pull request as ready for review November 27, 2020 03:02
@ebebbington ebebbington added Type: Major Merging this pull request results in a major version increment and removed WIP This item is a work in progress labels Nov 27, 2020
@ebebbington
Copy link
Member Author

ready for review now

}
}
// Connect websocket
this.socket = new WebSocket(debugUrl);
Copy link
Member

Choose a reason for hiding this comment

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

can we integrate wocket?

Copy link
Member Author

Choose a reason for hiding this comment

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

Wocket is a ws server right? Sinco just needs a ws client - or do you mean to use-wocket-client?

Copy link
Member

Choose a reason for hiding this comment

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

#19 (comment)

same question i have for both of these comments... what's the best place i can read more about this?

Copy link
Member Author

Choose a reason for hiding this comment

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

ill try get some info for you

@saragee3
Copy link
Member

saragee3 commented Dec 6, 2020

this is cool... what's the advantage of using sockets vs sub process?

@ebebbington
Copy link
Member Author

this is cool... what's the advantage of using sockets vs sub process?

So i heard from @caspervonb that using websockets is the better approach when connecting to headless chromium session, plus, there's a bug with sub process that essentially stops sinco from working - both reasons were why i switched to using web sockets, plus, using a sub process, we'd be running the headless browser as a repl (--repl), meaning it could only act as how the dev tools console does (no checking if a page has loaded, solely JS expressions)

@saragee3 saragee3 mentioned this pull request Dec 6, 2020
3 tasks
Copy link
Member

@saragee3 saragee3 left a comment

Choose a reason for hiding this comment

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

i mean.... LGTM! 😎 I made a ticket for eval too. feel free to comment on the ticket regarding func name!

@ebebbington
Copy link
Member Author

Woop woop! will wait for a review from @crookse or @Guergeiro then - after that... Sinco v1 here we come 😎

Copy link
Member

@Guergeiro Guergeiro left a comment

Choose a reason for hiding this comment

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

Nitpicking here... Is just that I feel that if we are already in an async function, why not just use await everywhere we can instead of relying on synchronous jobs?

src/headless_browser.ts Outdated Show resolved Hide resolved
src/utility.ts Show resolved Hide resolved
@ebebbington ebebbington merged commit 1a4b27a into master Jan 2, 2021
@ebebbington ebebbington deleted the v1 branch January 2, 2021 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Major Merging this pull request results in a major version increment
5 participants