Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(xychart): add ssr to xychart #1480

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

nagiek
Copy link

@nagiek nagiek commented Apr 26, 2022

Fixes #1478

馃殌 Enhancements

  • Allows data to be set upfront in as well as in in child Series components.
  • Fixes a chicken-and-egg issue where we delay rendering until data is passed into the children Series components.

Allows data to be set upfront in <XYChart> as well as in in child Series components. Fixes a chicken-and-egg issue where we delay rendering until data is passed into the children Series components.
@nagiek
Copy link
Author

nagiek commented Apr 27, 2022

@williaster quick ping for workflow approval, thanks!

@williaster
Copy link
Collaborator

Thanks for the PR @nagiek 馃檹 . My main concern with this approach is that two ways to pass data (to XYChart or to *Series) may cause confusion.

I just tried implementing the useComponentWillMount approach but the () => unregisterData(dataKeys) we currently return in useEffect for unmounting components is causing problems/I can't quite get it to work right. I'll keep tinkering a bit and think about whether there are any alternatives to your suggested approach.

@nagiek
Copy link
Author

nagiek commented May 3, 2022

No problem, I understand. I kept working on my version but I agree it鈥檚 a different technique.

What about splitting the existing useEffect call into two calls? One to useMemo containing registerData, and one to useEffect containing unregisterData?

@nagiek
Copy link
Author

nagiek commented May 16, 2022

Quick follow up, thank you!

@ThnxFredrik
Copy link

ThnxFredrik commented May 27, 2022

Hello! I鈥檓 suffering the same issue over here. How is the progress looking, can I help out some how?

@mscottford
Copy link

@nagiek Are you using this approach in production? I'd love to see SSR supported.

@mscottford
Copy link

@williaster

My main concern with this approach is that two ways to pass data (to XYChart or to *Series) may cause confusion.

Would a separate component make sense? Similar to how there are separate Animated and not-Animated components, there could be a ServerXYChart and a ClientXYChart, which could be aliased as just XYChart for backward compatibility. This way the different components could take different approaches to loading the data, and any duplicated functionality could be de-duplicated behind the scenes.

@pedroapfilho
Copy link

Any updates on this? Doing what @mscottford said, creating a ServerXYChart, would fix the issues that you commented about @williaster? Are these still issues on the current react version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[XYChart] SSR does not work on XYChart
5 participants