You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/01/20 06:45:41 UTC

[GitHub] [superset] rusackas opened a new pull request #12613: feat: Quick POC of rendering all charts in iframes on dashboards

rusackas opened a new pull request #12613:
URL: https://github.com/apache/superset/pull/12613


   ### SUMMARY
   <!--- Describe the change below, including rationale and design decisions -->
   Just a quick POC to show charts being rendered in iFrames. This code will become more important soon, and I'll give it more attention in terms of styling, but it should fix some potential security issues, and _maybe_ some performance issues (TBD). It needs a lot of styling help, but it seems plausible after doing some quick tire-kicking with this technique.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   ### TEST PLAN
   <!--- What steps should be taken to verify the changes -->
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] stale[bot] closed pull request #12613: feat: [WIP] Quick POC of rendering all charts in iframes on dashboards

Posted by GitBox <gi...@apache.org>.
stale[bot] closed pull request #12613:
URL: https://github.com/apache/superset/pull/12613


   


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] rusackas commented on pull request #12613: feat: [WIP] Quick POC of rendering all charts in iframes on dashboards

Posted by GitBox <gi...@apache.org>.
rusackas commented on pull request #12613:
URL: https://github.com/apache/superset/pull/12613#issuecomment-763978504


   Much to figure out here indeed, but I wanted to start fiddling around in the open, when time allows. Undoubtedly, this will require solving a few puzzles, but I hope it's not TOO painful. The library in use is maintained by some zendesk folk, who use it for their embedded widgets. The reason I was looking at that was originally for viz plugin sandboxing purposes. Previously any plugin code could have full access to the `window` object, which is full of security pitfalls. No big deal when they're _our_ plugins that we maintain, but in broadening scope to community contributed viz plugins, this was one of many areas to shore up. I'm hoping we can get some other benefits out of it too though, so consider this whole PR a big "tire kick". A feature flag sounds like a perfectly reasonable idea as long as the code is easy to keep isolated, so I'll pencil that in as part of the plan!


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] etr2460 commented on pull request #12613: feat: [WIP] Quick POC of rendering all charts in iframes on dashboards

Posted by GitBox <gi...@apache.org>.
etr2460 commented on pull request #12613:
URL: https://github.com/apache/superset/pull/12613#issuecomment-763779322


   Interesting concept, would be great to understand more about the reasons for this. One immediate thought however, is that separating all the charts into iframes might make inter-chart communication more difficult. Things like the filter box adding filters on the dashboard, or aligning tooltips between all charts on hover (a.la. what datadog does, not a current feature but has been discussed a bunch)


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] graceguo-supercat edited a comment on pull request #12613: feat: [WIP] Quick POC of rendering all charts in iframes on dashboards

Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #12613:
URL: https://github.com/apache/superset/pull/12613#issuecomment-763849557


   The impact of this change might be pretty big but hard to measure, especially for page load performance. 
   after this change, is dashboard perf logging still working? 
   Could you wrap iframe implementation in a **FEATURE FLAG**, so that we can compare and check perf impact?
   
   Agree with Erik's comment, how to handle intra-dashboard interactions? besides on filter boxes, i thought Superset will/should also implements _chart filter_ in the future.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] graceguo-supercat commented on pull request #12613: feat: [WIP] Quick POC of rendering all charts in iframes on dashboards

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #12613:
URL: https://github.com/apache/superset/pull/12613#issuecomment-763849557


   The impact of this change might be pretty big but hard to measure, especially for page load performance. 
   after this change, is dashboard perf logging still working? 
   Could you wrap iframe implementation in a **FEATURE FLAG**, so that we can compare and check perf impact?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] rusackas commented on pull request #12613: feat: [WIP] Quick POC of rendering all charts in iframes on dashboards

Posted by GitBox <gi...@apache.org>.
rusackas commented on pull request #12613:
URL: https://github.com/apache/superset/pull/12613#issuecomment-763978504


   Much to figure out here indeed, but I wanted to start fiddling around in the open, when time allows. Undoubtedly, this will require solving a few puzzles, but I hope it's not TOO painful. The library in use is maintained by some zendesk folk, who use it for their embedded widgets. The reason I was looking at that was originally for viz plugin sandboxing purposes. Previously any plugin code could have full access to the `window` object, which is full of security pitfalls. No big deal when they're _our_ plugins that we maintain, but in broadening scope to community contributed viz plugins, this was one of many areas to shore up. I'm hoping we can get some other benefits out of it too though, so consider this whole PR a big "tire kick". A feature flag sounds like a perfectly reasonable idea as long as the code is easy to keep isolated, so I'll pencil that in as part of the plan!


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] stale[bot] commented on pull request #12613: feat: [WIP] Quick POC of rendering all charts in iframes on dashboards

Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #12613:
URL: https://github.com/apache/superset/pull/12613#issuecomment-859200902


   This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue `.pinned` to prevent stale bot from closing the issue.
   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org