voidmatcha commented on code in PR #5266: URL: https://github.com/apache/zeppelin/pull/5266#discussion_r3405179619
########## zeppelin-web-angular/src/app/share/react-mount/react-remote-loader.service.ts: ########## @@ -0,0 +1,92 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { Injectable } from '@angular/core'; +import { environment } from '../../../environments/environment'; +import { AnyExposedModule } from './react-mount-handle'; + +interface RemoteContainer { + get<T>(key: string): Promise<() => T>; + init?: (shareScope: unknown) => Promise<void>; +} + +declare global { + interface Window { + reactApp?: RemoteContainer; + } +} + +@Injectable({ providedIn: 'root' }) +export class ReactRemoteLoaderService { + private containerPromise: Promise<RemoteContainer> | null = null; + private readonly modulePromises = new Map<string, Promise<AnyExposedModule>>(); + + loadContainer(): Promise<RemoteContainer> { + if (this.containerPromise) { + return this.containerPromise; + } + + this.containerPromise = new Promise<RemoteContainer>((resolve, reject) => { + if (window.reactApp) { + resolve(window.reactApp); + return; + } + + const script = document.createElement('script'); + script.src = environment.reactRemoteEntryUrl; + script.async = true; + script.onload = () => { + if (!window.reactApp) { + reject(new Error('window.reactApp not registered after script load')); + return; + } + resolve(window.reactApp); + }; + script.onerror = () => { + reject(new Error(`Failed to load React remote at ${script.src}`)); + }; Review Comment: ```suggestion script.onerror = () => { script.remove(); reject(new Error(`Failed to load React remote at ${script.src}`)); }; ``` Failed <script> tags are never removed, so each retry appends another dead tag. ########## zeppelin-web-angular/projects/zeppelin-react/src/components/paragraph/ParagraphFooter.css: ########## @@ -0,0 +1,17 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +.zeppelin-react-paragraph-footer { + color: rgba(0, 0, 0, 0.45); + font-size: 12px; + margin-top: 12px; +} Review Comment: ```suggestion } html.dark .zeppelin-react-paragraph-footer { color: rgba(255, 255, 255, 0.65); } ``` **Original** - master(Angular) <img width="1390" height="257" alt="Image" src="https://github.com/user-attachments/assets/3b4efaee-77db-4e5b-a937-47391969700d"; /> **As-Is** <img width="1390" height="257" alt="Image" src="https://github.com/user-attachments/assets/cd7290ce-c26c-4a01-a681-81999f226fed"; /> **To-Be** <img width="1390" height="257" alt="Image" src="https://github.com/user-attachments/assets/8da28de5-e305-4e18-b0f0-e878406515c9"; /> In dark mode the footer is nearly invisible. the color is hardcoded to the light-theme token, while the Angular footer it replaces adapts via theme-mixin (screenshots attached). ThemeService sets `.dark` on `<html>`. ########## zeppelin-web-angular/e2e/tests/notebook/paragraph/react-footer.spec.ts: ########## @@ -0,0 +1,104 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { expect, test } from '@playwright/test'; +import { + addPageAnnotationBeforeEach, + createTestNotebook, + PAGES, + performLoginIfRequired, + waitForNotebookLinks, + waitForZeppelinReady +} from '../../../utils'; + +test.describe('React Paragraph Footer', () => { + addPageAnnotationBeforeEach(PAGES.WORKSPACE.NOTEBOOK); + + let testNotebook: { noteId: string; paragraphId: string }; + + test.beforeEach(async ({ page }) => { + await page.goto('/#/'); + await waitForZeppelinReady(page); + await performLoginIfRequired(page); + await waitForNotebookLinks(page); + testNotebook = await createTestNotebook(page); + }); + + test('without reactFooter flag, Angular footer renders', async ({ page }) => { + const { noteId } = testNotebook; + + await page.goto(`/#/notebook/${noteId}`); + await waitForZeppelinReady(page); + + await expect(page.locator('[data-testid="angular-paragraph-footer"]').first()).toBeAttached({ timeout: 15000 }); + await expect(page.locator('[data-testid="react-paragraph-footer"]')).toHaveCount(0); + }); + + test('with reactFooter=true, React footer renders', async ({ page }) => { + const { noteId } = testNotebook; + + await page.goto(`/#/notebook/${noteId}?reactFooter=true`); + await waitForZeppelinReady(page); + + await expect(page.locator('[data-testid="react-paragraph-footer"]').first()).toBeAttached({ timeout: 15000 }); + await expect(page.locator('[data-testid="angular-paragraph-footer"]')).toHaveCount(0); + }); + + test('reactFooter=true preserves the paragraph query param', async ({ page }) => { + const { noteId, paragraphId } = testNotebook; + + await page.goto(`/#/notebook/${noteId}?paragraph=${paragraphId}&reactFooter=true`); + await waitForZeppelinReady(page); + + await expect(page).toHaveURL(/reactFooter=true/); + await expect(page).toHaveURL(new RegExp(`paragraph=${paragraphId}`)); + await expect(page.locator('[data-testid="react-paragraph-footer"]').first()).toBeAttached({ timeout: 15000 }); + }); + + test('when the remote fails to load, paragraphs fall back to the Angular footer', async ({ page }) => { + const { noteId } = testNotebook; + + // Simulate a dead remote: every remoteEntry.js request fails + await page.route('**/remoteEntry.js', route => route.abort()); + + await page.goto(`/#/notebook/${noteId}?reactFooter=true`); + await waitForZeppelinReady(page); + + // The loader rejection reaches each paragraph's onError, which flips + // reactFooterFailed and re-renders the Angular footer + await expect(page.locator('[data-testid="angular-paragraph-footer"]').first()).toBeAttached({ timeout: 15000 }); + await expect(page.locator('[data-testid="react-paragraph-footer"]')).toHaveCount(0); + }); + + test('navigating away during remoteEntry load does not throw', async ({ page }) => { + const { noteId } = testNotebook; + + // Delay remoteEntry.js to widen the destroy-while-loading window + await page.route('**/remoteEntry.js', async route => { + await new Promise(r => setTimeout(r, 1500)); + await route.continue(); + }); + + const consoleErrors: string[] = []; + page.on('pageerror', err => consoleErrors.push(err.message)); + + await page.goto(`/#/notebook/${noteId}?reactFooter=true`); + // Navigate away before the remote can possibly mount + await page.waitForTimeout(100); Review Comment: ```suggestion const remoteRequested = page.waitForRequest('**/remoteEntry.js'); await page.goto(`/#/notebook/${noteId}?reactFooter=true`); await remoteRequested; ``` If the remote loads within 100ms, the destroy-while-loading path isn't exercised yet the test still passes. ########## zeppelin-web-angular/e2e/tests/notebook/paragraph/react-footer.spec.ts: ########## @@ -0,0 +1,104 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { expect, test } from '@playwright/test'; +import { + addPageAnnotationBeforeEach, + createTestNotebook, + PAGES, + performLoginIfRequired, + waitForNotebookLinks, + waitForZeppelinReady +} from '../../../utils'; + +test.describe('React Paragraph Footer', () => { + addPageAnnotationBeforeEach(PAGES.WORKSPACE.NOTEBOOK); + + let testNotebook: { noteId: string; paragraphId: string }; + + test.beforeEach(async ({ page }) => { + await page.goto('/#/'); + await waitForZeppelinReady(page); + await performLoginIfRequired(page); + await waitForNotebookLinks(page); + testNotebook = await createTestNotebook(page); + }); + + test('without reactFooter flag, Angular footer renders', async ({ page }) => { + const { noteId } = testNotebook; + + await page.goto(`/#/notebook/${noteId}`); + await waitForZeppelinReady(page); + + await expect(page.locator('[data-testid="angular-paragraph-footer"]').first()).toBeAttached({ timeout: 15000 }); + await expect(page.locator('[data-testid="react-paragraph-footer"]')).toHaveCount(0); + }); + + test('with reactFooter=true, React footer renders', async ({ page }) => { + const { noteId } = testNotebook; + + await page.goto(`/#/notebook/${noteId}?reactFooter=true`); + await waitForZeppelinReady(page); + + await expect(page.locator('[data-testid="react-paragraph-footer"]').first()).toBeAttached({ timeout: 15000 }); + await expect(page.locator('[data-testid="angular-paragraph-footer"]')).toHaveCount(0); + }); + + test('reactFooter=true preserves the paragraph query param', async ({ page }) => { + const { noteId, paragraphId } = testNotebook; + + await page.goto(`/#/notebook/${noteId}?paragraph=${paragraphId}&reactFooter=true`); + await waitForZeppelinReady(page); + + await expect(page).toHaveURL(/reactFooter=true/); + await expect(page).toHaveURL(new RegExp(`paragraph=${paragraphId}`)); + await expect(page.locator('[data-testid="react-paragraph-footer"]').first()).toBeAttached({ timeout: 15000 }); Review Comment: ```suggestion await expect(page.locator('[data-testid="react-paragraph-footer-content"]').first()).toBeAttached({ timeout: 15000 }); ``` Same here ########## zeppelin-web-angular/e2e/tests/notebook/paragraph/react-footer.spec.ts: ########## @@ -0,0 +1,104 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { expect, test } from '@playwright/test'; +import { + addPageAnnotationBeforeEach, + createTestNotebook, + PAGES, + performLoginIfRequired, + waitForNotebookLinks, + waitForZeppelinReady +} from '../../../utils'; + +test.describe('React Paragraph Footer', () => { + addPageAnnotationBeforeEach(PAGES.WORKSPACE.NOTEBOOK); + + let testNotebook: { noteId: string; paragraphId: string }; + + test.beforeEach(async ({ page }) => { + await page.goto('/#/'); + await waitForZeppelinReady(page); + await performLoginIfRequired(page); + await waitForNotebookLinks(page); + testNotebook = await createTestNotebook(page); + }); + + test('without reactFooter flag, Angular footer renders', async ({ page }) => { + const { noteId } = testNotebook; + + await page.goto(`/#/notebook/${noteId}`); + await waitForZeppelinReady(page); + + await expect(page.locator('[data-testid="angular-paragraph-footer"]').first()).toBeAttached({ timeout: 15000 }); + await expect(page.locator('[data-testid="react-paragraph-footer"]')).toHaveCount(0); + }); + + test('with reactFooter=true, React footer renders', async ({ page }) => { + const { noteId } = testNotebook; + + await page.goto(`/#/notebook/${noteId}?reactFooter=true`); + await waitForZeppelinReady(page); + + await expect(page.locator('[data-testid="react-paragraph-footer"]').first()).toBeAttached({ timeout: 15000 }); Review Comment: ```suggestion await expect(page.locator('[data-testid="react-paragraph-footer-content"]').first()).toBeAttached({ timeout: 15000 }) ``` This asserts the Angular mount host (react-paragraph-footer), which is attached even if React never renders, so the test passes even if the React footer is broken. Assert the React-rendered node instead. ########## zeppelin-web-angular/src/app/share/react-mount/react-mount.directive.ts: ########## @@ -0,0 +1,146 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { Directive, ElementRef, Input, NgZone, OnChanges, OnDestroy, SimpleChanges } from '@angular/core'; +import { ReactRemoteLoaderService } from './react-remote-loader.service'; +import { + AnyExposedModule, + ReactExposedModule, + ReactHostCallbacks, + ReactMountHandle, + ReactProps +} from './react-mount-handle'; + +const isLegacyModule = (mod: AnyExposedModule, handleOrUnmount: unknown): handleOrUnmount is () => void => { + void mod; + return typeof handleOrUnmount === 'function'; +}; + +const wrapLegacyHandle = (unmount: () => void): ReactMountHandle => ({ + update: () => { + /* legacy modules don't support updates; no-op */ + }, + unmount +}); + +@Directive({ + selector: '[zeppelin-react-mount]', + standalone: false +}) +export class ReactMountDirective implements OnChanges, OnDestroy { + @Input('zeppelin-react-mount') module!: string; + @Input() reactProps: ReactProps & ReactHostCallbacks = {}; + + private latestProps: ReactProps & ReactHostCallbacks = {}; + private destroyed = false; + private loading = false; + private handle: ReactMountHandle | null = null; + private mountedModule: string | null = null; + + constructor( + private readonly host: ElementRef<HTMLElement>, + private readonly ngZone: NgZone, + private readonly loader: ReactRemoteLoaderService + ) {} + + ngOnChanges(changes: SimpleChanges): void { + this.latestProps = this.reactProps ?? {}; + + if (changes.module && !changes.module.firstChange && this.mountedModule) { + // Module swap after first mount is unsupported. Report via onError + // and otherwise leave the existing handle in place. + this.reportError( + new Error( + `ReactMountDirective: module input changed after mount ` + + `(from "${this.mountedModule}" to "${this.module}") — unsupported` + ) + ); + return; + } + + if (this.handle) { + this.ngZone.runOutsideAngular(() => { + try { + this.handle!.update(this.latestProps); + } catch (err) { + this.reportError(err); + } + }); + return; + } + + if (!this.loading && !this.destroyed && this.module) { + void this.startLoad(); + } + } + + ngOnDestroy(): void { + this.destroyed = true; + if (this.handle) { + try { + this.handle.unmount(); + } catch (err) { + this.reportError(err); + } + this.handle = null; + } + } + + private async startLoad(): Promise<void> { + this.loading = true; + const moduleKey = this.module; + try { + const mod = await this.loader.loadModule<AnyExposedModule>(moduleKey); + if (this.destroyed) { + return; + } + this.ngZone.runOutsideAngular(() => { + try { + const returned = (mod as ReactExposedModule).mount(this.host.nativeElement, this.latestProps); + if (isLegacyModule(mod, returned)) { + this.handle = wrapLegacyHandle(returned as unknown as () => void); + } else { + this.handle = returned as ReactMountHandle; + } + this.mountedModule = moduleKey; + } catch (err) { + this.handle = null; + this.reportError(err); + } + }); + } catch (err) { + this.reportError(err); + } finally { Review Comment: ```suggestion } catch (err) { if (!this.destroyed) { // mirror the L103 check on the success path this.reportError(err); } } finally { ``` `startLoad`'s outer `catch` misses the `destroyed` guard the success path has a load failing after navigation calls back into a destroyed host. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
