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

Binary protocol channeling over HTTP #319

Merged
merged 3 commits into from
Apr 1, 2022
Merged

Binary protocol channeling over HTTP #319

merged 3 commits into from
Apr 1, 2022

Conversation

1st1
Copy link
Member

@1st1 1st1 commented Mar 28, 2022

Requires the server to run with edgedb/edgedb#3682.


@jaclarke James, I hope this would unblock you to implement querying from the admin panel.

Some notes:

  1. I've only tested this running from the nodejs environment (not browser). The key limitation here is that node (pre 17.5) doesn't have native fetch(), so I'm using node-fetch to emulate it for testing purposes. When you run this in the browser, you might need to update FetchSocket.write() to properly cast emulated Buffer to something that browser's fetch() can understand (most likely ArrayBuffer).

  2. Every HTTP request essentially creates a new (temporary) binary connection on the server. Which means that the typical [parse] [execute] message sequence doesn't work!

    • When you execute [parse] it creates an anonymous statement.
    • The subsequent [execute] message doesn't see that anonymous statement, because the connection is already different.
    • The way to fix this is to use the [parse] [optimisticExecute] sequence, essentially not relying on anonymous statements at all.
    • I've fixed (in a hacky way) the .query() method to do just that and it works. However, iirc the studio uses the rawExecute method that would have to be updated to also use optimistic execute.
  3. Aside from the above it should work in the browser. I'll be completing some ToDos over the next couple of days, but you should be able to start playing with this.


ToDo

  • Proper state management in FetchSocket
  • Think about the API -- do we want createClient to automatically detect the browser environment and try to use fetch()? (this decision can wait at least until we think about enabling this feature beyond admin panel)

@jaclarke to test things from nodejs:

edgedb = require('./dist/index.node.js');

async function main() {
  con = edgedb.createFetchClient({host: 'localhost', port: 5656});
  console.log(await con.query('select 1'));
  console.log(await con.query('select {1, 2, 3}'));
}

main()

(don't forget to run yarn to install node-fetch@2)


James, lastly, please commit and push directly to this branch when you make this work in the browser.

@1st1 1st1 requested a review from jaclarke March 28, 2022 06:21
@1st1 1st1 force-pushed the fetch branch 3 times, most recently from f13b528 to 8d7f259 Compare April 1, 2022 06:32
@1st1 1st1 marked this pull request as ready for review April 1, 2022 06:35
@1st1
Copy link
Member Author

1st1 commented Apr 1, 2022

@jaclarke this should be ready for merge. Please review, push any changes you see necessary, and merge/release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants