You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@zeppelin.apache.org by "tbonelee (via GitHub)" <gi...@apache.org> on 2025/09/14 11:37:47 UTC
[PR] [ZEPPELIN-6327] Fix wrong order of the arguments for Visualization components [zeppelin]
tbonelee opened a new pull request, #5074:
URL: https://github.com/apache/zeppelin/pull/5074
### What is this PR for?
Visualizations are not working because the constructor arguments were passed in the wrong order.
Since using `any` made this error hard to detect, I replaced it with specific types.
### What type of PR is it?
Bug Fix
### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-6327
### Questions:
* Does the license files need to update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6327] Fix wrong order of the arguments for Visualization components [zeppelin]
Posted by "dididy (via GitHub)" <gi...@apache.org>.
dididy commented on PR #5074:
URL: https://github.com/apache/zeppelin/pull/5074#issuecomment-3292610192
I didn’t realize there could be such an edge case. From a quick look, I now understand that Angular allows calling constructors dynamically in that way, and in such cases, the order of the parameters must match the order in the constructor. From my quick review, it seems that there are no side effects from the changes I made that are similar to the issue caused here by my changes. But just to be safe, I’ll take a closer look.
One thing I’m curious about: I understand that `visualizationItem.componentFactoryResolver` was removed because it’s no longer needed in Angular 9. Is that correct? It looks like it’s still being used in **visualization-component-portal.ts** and **json-visualization.ts**, and based on my review, that’s probably because the project is still using Angular 8. I was wondering if there’s a plan to upgrade to Angular 9, and if not, what challenges are preventing it.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6327] Fix wrong order of the arguments for Visualization components [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee merged PR #5074:
URL: https://github.com/apache/zeppelin/pull/5074
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6327] Fix wrong order of the arguments for Visualization components [zeppelin]
Posted by "dididy (via GitHub)" <gi...@apache.org>.
dididy commented on code in PR #5074:
URL: https://github.com/apache/zeppelin/pull/5074#discussion_r2363234209
##########
zeppelin-web-angular/src/app/pages/workspace/share/result/result.component.ts:
##########
@@ -198,6 +200,7 @@ export class NotebookParagraphResultComponent implements OnInit, AfterViewInit,
}
private handleVisualizationBundles(bundles: HeliumVisualizationBundle[]): void {
+ console.log('bundles', bundles);
Review Comment:
It looks like a console.log was accidentally left in.
Sorry for the late reply. If it’s not actually being used, I agree that gradually removing Helium visualization interface that relies on Angular's dynamic rendering makes sense. I fully agree with reducing Angular dependencies, and the direction you suggested sounds good to me.
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6327] Fix wrong order of the arguments for Visualization components [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee commented on PR #5074:
URL: https://github.com/apache/zeppelin/pull/5074#issuecomment-3298618264
I missed that `componentFactoryResolver` could actually be passed into `JsonVisualization`. The reason I removed it was because no component in `src/app/visualizations` seemed to be using it, so I thought it was unused. For `JsonVisualization`, however, it seems better to restore it.
That said, I was also considering whether we should remove the new Helium visualization interface that relies on Angular's dynamic rendering. My reasoning is twofold:
1. The Helium visualization packages currently published on npm are still based on the old UI (AngularJS) visualization class, so I don't think the new interface is practically in use. (`JsonVisualization` exists, but I see it more as an example than something used in practice.)
2. Even if we do provide a new Helium visualization interface, I think it would be better if it could be implemented without depending on Angular. That way, we could reduce framework-specific dependencies in the future.
So my proposal is:
- Add an adapter for the old Helium visualization packages (#5006).
- Keep the new visualization logic only for the default visualizations for now (`src/app/visualizations`)
- If a new Helium visualization interface is needed later, provide a framework-agnostic class instead of one tied to Angular.
What do you think about this direction?
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6327] Fix wrong order of the arguments for Visualization components [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee commented on PR #5074:
URL: https://github.com/apache/zeppelin/pull/5074#issuecomment-3374964328
Merged into master and branch-0.12
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6327] Fix wrong order of the arguments for Visualization components [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee commented on PR #5074:
URL: https://github.com/apache/zeppelin/pull/5074#issuecomment-3374968755
Merged into master and branch-0.12
--
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: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org