-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[ZEPPELIN-6371] Convert published paragraph rendering to Micro Frontend(Angular to React) in New UI #5111
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
base: master
Are you sure you want to change the base?
Conversation
4d444be to
e1495e3
Compare
e1495e3 to
3570159
Compare
f41c0e3 to
2f2b8d9
Compare
|
Thank you for the proposal and the PR! First of all, I agree with the idea of migrating the framework from Angular to React. This change could help lower the barrier for front-end contributions, considering that we currently have few FE committers. However, since this is quite an epic change, I think it would be good to share this roadmap and gather feedback from Zeppelin developers first. Below are some examples from previous discussions for reference: |
|
Additional requests: if you have some time, could you extract the commits which fix the E2E tests into an separate PR? |
### What is this PR for? #5111 (comment) #5108 After upgrading Playwright to the latest version(1.53.2 to 1.56.1), several test cases started failing on WebKit. The issue was resolved by rolling back to the previous minor version(1.55.1). [related issue] microsoft/playwright#37766 ### What type of PR is it? Hot Fix ### Todos ### What is the Jira issue? ### How should this be tested? ### Screenshots (if appropriate) ### Questions: * Does the license files need to update? No * Is there breaking changes for older versions? No * Does this needs documentation? No Closes #5115 from dididy/fix/e2e. Signed-off-by: ChanHo Lee <chanholee@apache.org>
### What is this PR for? #5111 (comment) #5108 After upgrading Playwright to the latest version(1.53.2 to 1.56.1), several test cases started failing on WebKit. The issue was resolved by rolling back to the previous minor version(1.55.1). [related issue] microsoft/playwright#37766 ### What type of PR is it? Hot Fix ### Todos ### What is the Jira issue? ### How should this be tested? ### Screenshots (if appropriate) ### Questions: * Does the license files need to update? No * Is there breaking changes for older versions? No * Does this needs documentation? No Closes #5115 from dididy/fix/e2e. Signed-off-by: ChanHo Lee <chanholee@apache.org> (cherry picked from commit bf62a2a) Signed-off-by: ChanHo Lee <chanholee@apache.org>
0889582 to
cd54756
Compare
| await page.goto(reactModeUrl); | ||
| await waitForZeppelinReady(page); | ||
|
|
||
| // URL 이동 완료까지 명시적으로 대기 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a Korean comment.
(I haven't started the review yet, just leaving a quick note.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the hard work!
I've pushed a few fixes for type errors, incorrect type definitions that caused the graph.mode value to be read incorrectly, and an issue where table header names were omitted when exporting to an xlsx file.
To address the incorrect type definitions, I replaced the types defined in zeppelin-react with the ones already defined in zeppelin-sdk.
You can check the changes here:
https://github.com/tbonelee/zeppelin/commits/PR_5111-review/
Please take a look and let me know if these changes make sense.
Lastly, it would be great if you could run Prettier once as well. It would add new lines at the EOF automatically.
Details
- This fixes type incompatibilies which cause type errors. You could check fixes by running
npx tsc --noEmitat the root of thezeppelin-reactbefore and after this commit.
- This fixes wrong use of Zeppelin related types.
- It also fixes the bug which could not set current mode in
TableVisualization
- Fixed a minor type error.
- Fixed an issue where table header names were omitted.
- Replace
.json_to_sheet()with.aoa_to_sheet().
| test('should enter published paragraph in React mode via URL with react=true', async ({ page }) => { | ||
| await test.step('Given I navigate to React mode URL', async () => { | ||
| const reactModeUrl = `/#/notebook/${testNotebook.noteId}/paragraph/${testNotebook.paragraphId}?react=true`; | ||
| await page.goto(reactModeUrl); | ||
| await waitForZeppelinReady(page); | ||
|
|
||
| await page.waitForURL(`**/${testNotebook.noteId}/paragraph/${testNotebook.paragraphId}*`, { | ||
| timeout: 15000 | ||
| }); | ||
| }); | ||
|
|
||
| await test.step('Then React mode should be active', async () => { | ||
| const currentUrl = page.url(); | ||
| expect(currentUrl).toContain('react=true'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's no special handling in place, navigating to a URL that includes query parameters will always result in those parameters being present in the final URL. In that case, wouldn't this test always pass regardless of whether React mode is actually active?
It may be better to either remove this test or revise it to validate React mode using a different, more reliable indicator.
| // Only clear output if result exists | ||
| if (await paragraphResult.isVisible()) { | ||
| const settingsButton = paragraphElement.locator('a[nz-dropdown]'); | ||
| await settingsButton.click(); | ||
|
|
||
| const clearOutputButton = page.locator('li.list-item:has-text("Clear output")'); | ||
| await clearOutputButton.click(); | ||
| await expect(paragraphResult).toBeHidden(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think test scenarios should be as deterministic as possible.
Since beforeEach creates a new notebook, we may not need a conditional if block here to clear the output.
It should be sufficient to assert that the paragraph result is hidden with something like expect(paragraphResult).toBeHidden().
| showIcon | ||
| /> | ||
| ); | ||
| }; No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new line at the EOF is needed.
| </Typography.Title> | ||
| </div> | ||
| ); | ||
| }; No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new line at the EOF is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should highlight code blocks like the Angular version.
Please check
zeppelin/zeppelin-web-angular/src/app/pages/workspace/share/result/result.component.ts
Line 319 in bf62a2a
| renderHTML(): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should handle ANSI color escape codes. See
zeppelin/zeppelin-web-angular/src/app/pages/workspace/share/result/result.component.ts
Line 367 in bf62a2a
| renderText(): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that, at this point, this doesn’t necessarily need to be a React custom hook and could simply be implemented as a regular utility function.
Do you have any plans for it to hold state or anything similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'd better add a .gitignore file inside zeppelin-react directory to ignore dist and node_modules in zeppelin-react.
What is this PR for?
Micro Frontend Migration(Angular to React) Proposal
Summary
Changes
1. React Micro-Frontend Project Setup
projects/zeppelin-react/.2. Component Implementation
New React Components:
PublishedParagraph: Main entry point for published paragraph rendering.SingleResultRenderer: Template for rendering single paragraph results.Renderers:
HTMLRenderer: Renders HTML content with sanitization.TextRenderer: Renders plain text with ANSI support.ImageRenderer: Renders image outputs.Visualizations:
TableVisualization: Table rendering with sorting, filtering, and export.VisualizationControls: Control panel for table operations.Common Components:
Loading: Loading state indicator.Empty: Empty state display.3. Angular Integration
paragraph.component.ts: Added React widget loading logic via Module Federation.paragraph.component.html: Added React container element.environment.ts/environment.prod.ts: AddedreactRemoteEntryUrlconfiguration.http://localhost:3001/remoteEntry.js/assets/react/remoteEntry.js4. Build Configuration
angular.json: Copy React build output to/assets/react/.webpack.config.js: Configured Module Federation plugin:publicPathproxy.conf.js: Updated proxy configuration.5. Package
License
This PR uses several open-source libraries. The
xlsx(v0.18.5) andtypescript(v4.6.4) packages are licensed under Apache-2.0, while all other dependencies and devDependencies (such asreact,react-dom,antd,@ant-design/icons, etc.) are licensed under MIT. The MIT license is more permissive than Apache-2.0, so including MIT-licensed packages does not violate Apache-2.0 terms. All packages may be used commercially, and license notices should be included when distributing the project.Technical Details
Module Federation Configuration
Usage
/notebook/{noteId}/paragraph/{paragraphId}?react=trueWhat type of PR is it?
Improvement
Todos
What is the Jira issue?
ZEPPELIN-6371
How should this be tested?
// Start Zeppelin Server ./mvnw clean install -DskipTests ./mvnw clean package -DskipTests ./bin/zeppelin-daemon.sh start // Start Zeppelin New UI Client cd zeppelin-web-angular nvm use npm i npm run startTextRenderer
http://localhost:4200/#/notebook/2EYDJKFFY/paragraph/20180118-122136_1299905608?react=true
TableVisualization
http://localhost:4200/#/notebook/2EYDJKFFY/paragraph/20180118-122136_1299905608?react=true
ImageRenderer
http://localhost:4200/#/notebook/2F1S9ZY8Z/paragraph/20180117-220535_590781730?react=true
HTMLRenderer - Table
http://localhost:4200/#/notebook/2F1S9ZY8Z/paragraph/paragraph_1580885453474_1167659991?react=true
HTMLRenderer - Script(Bokeh JS)
http://localhost:4200/#/notebook/2F1S9ZY8Z/paragraph/paragraph_1580885707198_-1652524072?react=true
Screenshots (if appropriate)
Questions: