From a9369b1374823987c5364c3f34d0f9920f01716a Mon Sep 17 00:00:00 2001 From: Will Chen Date: Sun, 27 Oct 2024 21:28:45 -0700 Subject: [PATCH] Fix race condition in processing commands (navigate) (#1065) --- mesop/examples/query_params.py | 14 ++++++++++++ mesop/tests/e2e/query_params_test.ts | 27 +++++++++++++++++++---- mesop/web/src/services/channel.ts | 32 +++++++++++++++++++++++----- mesop/web/src/shell/shell.ts | 4 ++-- 4 files changed, 66 insertions(+), 11 deletions(-) diff --git a/mesop/examples/query_params.py b/mesop/examples/query_params.py index 32d5ba975..bdb47426c 100644 --- a/mesop/examples/query_params.py +++ b/mesop/examples/query_params.py @@ -105,7 +105,21 @@ def increment_by_navigate(e: me.ClickEvent): def page_2(): me.text(f"query_params(page_2)={me.query_params}") me.button("Navigate back", on_click=navigate_back) + me.button("Navigate page 3", on_click=navigate_page_3) + + +def on_load_page_3(e: me.LoadEvent): + me.query_params["on_load_page_3"] = "loaded" + + +@me.page(path="/examples/query_params/page_3", on_load=on_load_page_3) +def page_3(): + me.text(f"query_params(page_3)={me.query_params}") def navigate_back(e: me.ClickEvent): me.navigate("/examples/query_params", query_params=me.query_params) + + +def navigate_page_3(e: me.ClickEvent): + me.navigate("/examples/query_params/page_3") diff --git a/mesop/tests/e2e/query_params_test.ts b/mesop/tests/e2e/query_params_test.ts index 1176fbb15..1b5bf7ce1 100644 --- a/mesop/tests/e2e/query_params_test.ts +++ b/mesop/tests/e2e/query_params_test.ts @@ -8,6 +8,20 @@ test('query_param: on_load hook', async ({page}) => { await expect(page).toHaveURL(/\?on_load=loaded$/); }); +test('query_param: navigate then on_load hook', async ({page}) => { + await page.goto('/examples/query_params/page_2'); + await page + .getByRole('button', { + name: 'Navigate page 3', + exact: true, + }) + .click(); + expect(await page.getByText('query_params(page_3)=').textContent()).toEqual( + `query_params(page_3)={'on_load_page_3': ['loaded']}`, + ); + await expect(page).toHaveURL(/page_3\?on_load_page_3=loaded$/); +}); + test('query_param: load with query param in URL', async ({page}) => { await page.goto('/examples/query_params?a=1'); expect(await page.getByText('query_params={').textContent()).toEqual( @@ -87,15 +101,18 @@ test('query_param: increment query param ', async ({page}) => { await expect( page.getByText(`query_params={'on_load': ['loaded'], 'counter': ['1']}`), ).toBeVisible(); - await expect(page).toHaveURL(/\?on_load=loaded&counter=1/); - + let url = new URL(page.url()); + expect(url.searchParams.get('on_load')).toBe('loaded'); + expect(url.searchParams.get('counter')).toBe('1'); await page .getByRole('button', {name: 'increment query param directly'}) .click(); await expect( page.getByText(`query_params={'on_load': ['loaded'], 'counter': ['2']}`), ).toBeVisible(); - await expect(page).toHaveURL(/\?on_load=loaded&counter=2/); + url = new URL(page.url()); + expect(url.searchParams.get('on_load')).toBe('loaded'); + expect(url.searchParams.get('counter')).toBe('2'); await page .getByRole('button', {name: 'increment query param by navigate'}) @@ -103,7 +120,9 @@ test('query_param: increment query param ', async ({page}) => { await expect( page.getByText(`query_params={'on_load': ['loaded'], 'counter': ['3']}`), ).toBeVisible(); - await expect(page).toHaveURL(/\?on_load=loaded&counter=3/); + url = new URL(page.url()); + expect(url.searchParams.get('on_load')).toBe('loaded'); + expect(url.searchParams.get('counter')).toBe('3'); }); test('query_param: delete all query params ', async ({page}) => { diff --git a/mesop/web/src/services/channel.ts b/mesop/web/src/services/channel.ts index 1bc367128..9b291c5ed 100644 --- a/mesop/web/src/services/channel.ts +++ b/mesop/web/src/services/channel.ts @@ -36,7 +36,7 @@ interface InitParams { jsModules: readonly string[], ) => void; onError: (error: ServerError) => void; - onCommand: (command: Command) => void; + onCommand: (command: Command) => Promise; } export enum ChannelStatus { @@ -64,6 +64,8 @@ export class Channel { private queuedEvents: (() => void)[] = []; private hotReloadBackoffCounter = 0; private hotReloadCounter = 0; + private commandQueue: Command[] = []; + private commandQueuePromise: Promise | undefined; // Client-side state private overridedTitle = ''; @@ -237,7 +239,7 @@ export class Channel { }; } - private handleUiResponse( + private async handleUiResponse( request: UiRequest, uiResponse: UiResponse, initParams: InitParams, @@ -283,9 +285,8 @@ export class Channel { const rootComponent = uiResponse.getRender()!.getRootComponent()!; const componentDiff = uiResponse.getRender()!.getComponentDiff()!; - for (const command of uiResponse.getRender()!.getCommandsList()) { - onCommand(command); - } + this.commandQueue.push(...uiResponse.getRender()!.getCommandsList()); + await this.processCommandQueue(onCommand); const title = this.overridedTitle || uiResponse.getRender()!.getTitle(); if (title) { @@ -354,6 +355,27 @@ export class Channel { } } + private async processCommandQueue( + onCommand: (command: Command) => Promise, + ) { + if (this.commandQueuePromise) { + return this.commandQueuePromise; + } + let resolveHandle!: () => void; + this.commandQueuePromise = new Promise((resolve) => { + resolveHandle = resolve; + }); + try { + while (this.commandQueue.length > 0) { + const command = this.commandQueue.shift()!; + await onCommand(command); + } + } finally { + this.commandQueuePromise = undefined; + resolveHandle(); + } + } + dispatch(userEvent: UserEvent) { // Every user event should have an event handler, // except for the ones below: diff --git a/mesop/web/src/shell/shell.ts b/mesop/web/src/shell/shell.ts index 54ce740e6..8d8770861 100644 --- a/mesop/web/src/shell/shell.ts +++ b/mesop/web/src/shell/shell.ts @@ -115,13 +115,13 @@ export class Shell { this.componentConfigs = componentConfigs; } }, - onCommand: (command) => { + onCommand: async (command) => { if (command.hasNavigate()) { const url = command.getNavigate()!.getUrl()!; if (url.startsWith('http://') || url.startsWith('https://')) { window.location.href = url; } else { - this.router.navigateByUrl(command.getNavigate()!.getUrl()!); + await this.router.navigateByUrl(command.getNavigate()!.getUrl()!); this.channel.resetOverridedTitle(); } } else if (command.hasScrollIntoView()) {