Skip to content

fix: Ensure WebsocketPolyfill always has the latest session state and version #6217

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.

Sign up for GitHub

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

Merged
juliusknorr merged 1 commit into from
Aug 19, 2024
Merged

fix: Ensure WebsocketPolyfill always has the latest session state and version #6217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/components/Editor.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,7 @@ export default {
},

onSync({ steps, document }) {
this.hasConnectionIssue = !this.$providers[0].wsconnected || this.$syncService.pushError > 0
this.hasConnectionIssue = this.$syncService.backend.fetcher === 0 || !this.$providers[0].wsconnected || this.$syncService.pushError > 0
Copy link
Member Author

@juliusknorr juliusknorr Aug 16, 2024

Choose a reason for hiding this comment

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

This callback is triggering for either a successful sync or push request so we need this as a workaround way to check if the other request currently has issues.

this.$nextTick(() => {
this.emit('sync-service:sync')
})
Expand Down
27 changes: 16 additions & 11 deletions src/services/SyncService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,19 @@ class SyncService {
return this.#connection && !this.#connection.isClosed
}

get connectionState() {
if (!this.#connection || this.version === undefined) {
return null
}
return {
...this.#connection.state,
version: this.version,
}
}

async open({ fileId, initialSession }) {
if (this.hasActiveConnection) {
// We're already connected.
return
return this.connectionState
}
const connect = initialSession
? Promise.resolve(new Connection({ data: initialSession }, {}))
Expand All @@ -106,19 +115,15 @@ class SyncService {
this.#connection = await connect
if (!this.#connection) {
// Error was already emitted in connect
return
return null
}
this.backend = new PollingBackend(this, this.#connection)
this.version = this.#connection.docStateVersion
this.baseVersionEtag = this.#connection.document.baseVersionEtag
this.emit('opened', {
...this.#connection.state,
version: this.version,
})
this.emit('loaded', {
...this.#connection.state,
version: this.version,
})
this.emit('opened', this.connectionState)
this.emit('loaded', this.connectionState)

return this.connectionState
}

startSync() {
Expand Down
17 changes: 11 additions & 6 deletions src/services/WebSocketPolyfill.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,11 @@ export default function initWebSocketPolyfill(syncService, fileId, initialSessio
this.#notifyPushBus?.on('notify_push', this.#onNotifyPush.bind(this))
this.url = url
logger.debug('WebSocketPolyfill#constructor', { url, fileId, initialSession })
if (syncService.hasActiveConnection) {
setTimeout(() => this.onopen?.(), 0)
}
this.#registerHandlers({
opened: ({ version, session }) => {
this.#version = version
logger.debug('opened ', { version, session })
this.#version = version
this.#session = session
this.onopen?.()
},
loaded: ({ version, session, content }) => {
logger.debug('loaded ', { version, session })
Expand All @@ -60,7 +56,16 @@ export default function initWebSocketPolyfill(syncService, fileId, initialSessio
}
},
})
syncService.open({ fileId, initialSession })

syncService.open({ fileId, initialSession }).then((data) => {
if (syncService.hasActiveConnection) {
Copy link
Member Author

@juliusknorr juliusknorr Aug 16, 2024

Choose a reason for hiding this comment

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

I decided to refactor this slightly as we then can just always rely on the open method to return the relevant data, in both first connects and reconnects

const { version, session } = data
this.#version = version
this.#session = session

this.onopen?.()
}
})
}

#registerHandlers(handlers) {
Expand Down
17 changes: 10 additions & 7 deletions src/tests/services/WebsocketPolyfill.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,21 +8,21 @@ import initWebSocketPolyfill from '../../services/WebSocketPolyfill.js'
describe('Init function', () => {

it('returns a websocket polyfill class', () => {
const syncService = { on: jest.fn(), open: jest.fn() }
const syncService = { on: jest.fn(), open: jest.fn(() => Promise.resolve({ version: 123, session: {} })) }
const Polyfill = initWebSocketPolyfill(syncService)
const websocket = new Polyfill('url')
expect(websocket).toBeInstanceOf(Polyfill)
})

it('registers handlers', () => {
const syncService = { on: jest.fn(), open: jest.fn() }
const syncService = { on: jest.fn(), open: jest.fn(() => Promise.resolve({ version: 123, session: {} })) }
const Polyfill = initWebSocketPolyfill(syncService)
const websocket = new Polyfill('url')
expect(syncService.on).toHaveBeenCalled()
})

it('opens sync service', () => {
const syncService = { on: jest.fn(), open: jest.fn() }
const syncService = { on: jest.fn(), open: jest.fn(() => Promise.resolve({ version: 123, session: {} })) }
const fileId = 123
const initialSession = { }
const Polyfill = initWebSocketPolyfill(syncService, fileId, initialSession)
Expand All @@ -33,7 +33,7 @@ describe('Init function', () => {
it('sends steps to sync service', async () => {
const syncService = {
on: jest.fn(),
open: jest.fn(),
open: jest.fn(() => Promise.resolve({ version: 123, session: {} })),
sendSteps: async getData => getData(),
}
const queue = [ 'initial' ]
Expand All @@ -51,9 +51,10 @@ describe('Init function', () => {
})

it('handles early reject', async () => {
jest.spyOn(console, 'error').mockImplementation(() => {})
const syncService = {
on: jest.fn(),
open: jest.fn(),
open: jest.fn(() => Promise.resolve({ version: 123, session: {} })),
sendSteps: jest.fn().mockRejectedValue('error before reading steps in sync service'),
}
const queue = [ 'initial' ]
Expand All @@ -69,9 +70,10 @@ describe('Init function', () => {
})

it('handles reject after reading data', async () => {
jest.spyOn(console, 'error').mockImplementation(() => {})
const syncService = {
on: jest.fn(),
open: jest.fn(),
open: jest.fn(() => Promise.resolve({ version: 123, session: {} })),
sendSteps: jest.fn().mockImplementation( async getData => {
getData()
throw 'error when sending in sync service'
Expand All @@ -90,9 +92,10 @@ describe('Init function', () => {
})

it('queue survives a close', async () => {
jest.spyOn(console, 'error').mockImplementation(() => {})
const syncService = {
on: jest.fn(),
open: jest.fn(),
open: jest.fn(() => Promise.resolve({ version: 123, session: {} })),
sendSteps: jest.fn().mockImplementation( async getData => {
getData()
throw 'error when sending in sync service'
Expand Down