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

Misc improvements #344

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Misc improvements #344

wants to merge 8 commits into from

Conversation

macjuul
Copy link
Contributor

@macjuul macjuul commented Sep 25, 2024

  • Refactored file names and imports

  • Enforce file name format

  • Initial work on reconnect support

  • I have read the Contributing Guidelines

@ntorrey
Copy link

ntorrey commented Sep 25, 2024

Yay for reconnect support! 🎉

@phisch
Copy link

phisch commented Oct 21, 2024

@macjuul We just ran into an issue of our backend losing the connection to the database, and the backend had no way to even tell us about that. Anything using queries was just stuck indefinitely. We'd be super stoked about reconnect support. But is there anything we can do in the meantime until this gets merged? Even just being able to have a timeout that would let us know the connection is lost would be a big improvement.

@macjuul
Copy link
Contributor Author

macjuul commented Oct 21, 2024

@macjuul We just ran into an issue of our backend losing the connection to the database, and the backend had no way to even tell us about that. Anything using queries was just stuck indefinitely. We'd be super stoked about reconnect support. But is there anything we can do in the meantime until this gets merged? Even just being able to have a timeout that would let us know the connection is lost would be a big improvement.

We are still debating on the internal implementation of a reconnect system, however in Surrealist I implemented this manually by subscribing to disconnect events and instantiating a new connection. Until official support is available this is likely the best approach

@phisch
Copy link

phisch commented Oct 21, 2024

@macjuul Thanks for letting me know. But that would only work using rust, right? I don't see a way to subscribe to events in the TS SDK. (Hopefully I'm just blind)

Edit: I just saw there is .emitter on the connection. Not documented, but it looks like that would do it.

Edit 2: Yep, this seems to be that:

connection.emitter.subscribe("disconnected", async () => {
    // whatever
});

@ntorrey
Copy link

ntorrey commented Nov 22, 2024

@macjuul Ooh, I never knew there was an .emitter to hook in to! I don't know if you're able, but could you point me in the right direction where you implement the manual reconnection in surrealist? I'm banging my head against the wall trying to figure it out. Or might you be able to share your implementation @phisch ? Thanks for the help and sorry for the bother!

@phisch
Copy link

phisch commented Nov 22, 2024

@ntorrey I can't share my implementation, but something along the lines of this should get you started:

const connection: Surreal = new Surreal();

async function connect() {
	await connection.connect(env.SURREALDB_URL, {
		namespace: env.SURREALDB_NAMESPACE,
		database: env.SURREALDB_DATABASE_NAME,
	});
}

connection.emitter.subscribe(ConnectionStatus.Connected, async () => {
	console.log("Connected to database!");
});

connection.emitter.subscribe(ConnectionStatus.Disconnected, async () => {
	connect();
});

connect();

I also handle browser online/offline state changes, have a connect retry-strategy with backoff in there and some more. But this should be a working start.

@ntorrey
Copy link

ntorrey commented Nov 23, 2024

@phisch Thank you for the response! I started tinkering and have more questions - I guess this isn't really the place for this, so I'll just ask on discord. Thanks again though!

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.

4 participants