Open cliffordfajardo opened 2 years ago
@pomelo-nwu,@zxc0328
Hi @Blakko @timlrx @Anderson-Liu @budlion @niknbr @CodesAreHonest @peironggg
I've noticed some of you have contributed or mentioned using graphin
so I've included many of you in this discussion.
Do you have any opinions on this idea of a useGraphin()
hook?
Did any of you find the existing API limiting due to graphin
not exposing something like a GraphinProvider
component to wrap your component?
@cliffordfajardo Thank you for your suggestions, we will think carefully and give some solutions. If you want, you can add my dingtalk and talk about it in detail
Below is another use case example of how having a useGraphin
hook and a <GraphinProvider>
is helpful.
Imagine I want to place a search component above my <Graphin>
component like this:
Currently this is not possible because the Graphin.Provider only exists inside of <Graphin>
@pomelo-nwu - I've created a codesandbox demonstrating real usage of the useGraphin
hook for the searching the graph example from the image above.
I was able to create a prototype of the desired behavior, without modifying graphin's internal code.
I had to create a wrapper component (see GrapinProvider.tsx
inside the codesanbox link below)
I would say it is a good non-hacky abstraction. Ideally we wouldn't need the wrapper component, but it could serve lots of people well in the meantime before it makes it into the core Graphin library/
If the proposed code changes for useGraphin
hook does make it into the @antv/graphin
library one day, people using the example abstraction (GraphnProvider.tsx
) I provided in the code sandbox wouldn't need to change almost anything when the GraphinProvider
and useGraphin
is exposed from the real library
Concerning what you mentioned on Graphin's DingTalk channel:
The reason why we didn't disclose the GraphinProvider to users was that when we designed it at that time, for the out-of-the-box use of components, for many developers, we added a layer of Provider writing, it may make them feel that the threshold for getting started is very high. At the same time, we generally use React.createPortal to solve the hierarchy problem of components, to ensure that all components are placed inside
I believe it should still be possible to keep the existing behavior of passing "behaviors" inside the
Ideally though the way to make this work in the real @antv/graphin
library is to convert the <Graphin>
component to a Functional component instead of it remaining a Class component.
Graphin.tsx
to a Function component's for a good foundation to build on.Tips for navigating the codesanbox:
GraphinProvider.tsx
<Graphin>
component, so now I can use the useGraphin
hook inside of the GraphinProvider.tsx
fileSetupGraphinHook.ts
<Graphin>
to finish loading and then it updates the GraphinProvider
by passing it the GraphinContextData
Note:
To prevent destructive upgrades my plan is to:
Hi, I'm also interested in 'useGraphin' hook. Features such as isReady, APIs and etc. are so effective.
But in my opinion, it should be done with fewer side effects for graphin. So I came up with an idea that uses a global "Graphin pool" for Graphin hooks.
For users who wants to useGraphin, they only need to append a global unique id to Graphin component, like \<Graphin id="graph-1" \/> and it can useGraphin("graph-1") anywhere.
Instance of Graphin will register into Graphin pool when init and remove when destroy, with register event, context will directly avaliable for useGraphin. (It's like a pub/sub model), and for users, they don't need to know graphin context, only change they need to know is that they can assign id to graphin instance and use graphin api throgh id anywhere they need.
What's your opinion about this pattern, looking forwards to your reply~
Hi @mxz96102 - I will share some thought's later today exploring concerning your idea.
Features such as isReady, APIs and etc. are so effective.
I agree!
It's a nice idea! Currently for motif, we are just storing the ref from Graphin and passing it through to the other widget components using a provider. The other components can then useContext
and have access to the full G6 api.
A useGraphin
would help simplify things.
Hi @mxz96102
But in my opinion, it should be done with fewer side effects for graphin
I agree that the changes that are made to Graphin should not dramatically change the usage of the library. @mxz96102 would you be able to share an example of what the code looks like in practice via a codesanbox, some visual or some pseudo code? I just want to make sure I understand your idea 😄
For this to Graphin Pool idea to work we would probably would need to write some code inside the useGraphin hook that searches the DOM for the element on the page with id="graph-1".
useGraphin('..')
is that useGraphin
will get called before the Thanks for bringing up the idea of multiple <Graphin>
components on the same page. This is a real use case & I'm actually doing this at work for page I built 🙂
With all that said, if we converted the <Graphin>
from a Class component to a Function component & created the GraphinProvider.tsx
file we wouldn't need a Graphin pool & globals (this makes testing harder) & we wouldn't run into the tricky scenarios I mentioned before.
Example of multiple Graphin components on a page below:
GraphinProvider
is unique, so the useGraphin
calls used inside the GraphinProvider
will only grab data from the <Graphin>
component scope inside the <GraphinProvider>
const Page = () => {
return (
<>
<GraphinProvider>
useGraphin() here will have access to the GraphinProvider's context, which is connected to the Graphin Component be low it
<Graphin></Graphin>
</GraphinProvider>
<GraphinProvider>
useGraphin() here will grab data from the GraphinContext inside the GraphinProvider that is directly above it only (this is normal out of the box react behavior)
<Graphin></Graphin>
</GraphinProvider>
</>
)
}
For the longevity of the Graphin project, I do believe its in the best interest of the library & it's community to migrate away from having the Graphin.tsx
file be a Class component and to rather have it be a Function component for a few reasons:
@mxz96102 I liked the concept of the global pool 💯 , but the technical implementation & limitations it brings in the near future I think are not worth it compared to the approach of having an exportable <GraphinProvider>
which is the more idiomatic React approach
This approach of creating an exportable GraphinProvider
in order to help us achieve the useGraphin
hook would still allow you to pass in behavior components inside <Graphin>
<Graphin
data={graphData}
layout={{ type: ".." }}
>
// you would still be able to pass in Behaviors with the new proposed changes (backwards compatible)
<SetupUseGraphinHook />
<ZoomCanvas />
</Graphin>
Updated smaller codesandbox of using useGraphin
with multiple Graph's on the page
@cliffordfajardo I came up with an id sample here and just extend the component, hope can help you in someway.
@mxz96102 - can you share more about your thoughts on the advantages / disadvantages of this approach versus other alternatives we've talked about in this discussion?
Genuinely, I am just curious and would like understand your idea better 🙏
I don't think we should continue using or relying on Class components, but rather we should work towards modernizing the codebase incrementally without making drastic changes).
Open source maintenance & development can hard & I think this path of using Function components will make current & future features easier to maintain and make it easier for new people to contribute to the codebase with their limited time outside of work to contribute to open source since they won't need to worry about learning 2 styles of code (Class vs Function components)
React 18 is releasing with lots of new features we would not be able to take advantage of:
With the Graphin pool approach, we are introducing a new concept of a pool, and having to manage that pool; I know its not much, but with the GraphinProvider
approach I've discussed above, we are not introducing new concepts into the codebase. Its really about 2 key things:
Graphin component
(line 433) a exportable valueAll the existing behavior will still just work 😄
Sorry if I wasn't clear, next time I will make a much smaller codesanbox like you
I agree that deprecate class component will embrace more features of React 18, but for now, it might not be done in this version, considering that some of the users still depend on class lifecycle to do something, in this version using id might be enough for users, and for break change (like converting Graphin to function component), may happen in a new version like Graphin 3?
Hi Graphin team & community, I would like your opinion on this feature proposal for the library.
Problem Statement
Currently, there is no way to access the
GraphinContext
data outside of the<Graphin>
component. This is because@antv/graphin
does not expose as an exportableGraphinProvider
component.Example / Scenario 1
Imagine I have a component called
<NetworkTopologyViewer>
which is responsible for setting up my<Graphin>
component along with custom behaviorThis is a live code example of what I'm trying to achieve above, which is to define a
handleClick
event for when a node in my graph gets clicked.Proposal
Expose a
useGraphin
hook, which allows us get & set data from theGraphinContext
.Benefits
GraphinContext
since theuseGraphin()
hook can get or set data from theGraphinContext
useGraphin()
hook inside of other hooks custom hooks.useGrapin()
.useGraphin
hook, I can access all the data I need from any component wrapped by theGraphinProvider
.Steps required to implement
useGraphin
in@antv/graphin
GraphinProvider
component & make it exportable to allow user's to wrap their component(s) with `GraphinProviderGraphinProvider
the data will be empty, but after<Graphin>
component loads, it will update theGraphinProvider
's context data// ---------OR initialize with data -------------- const graphinContextData = {...};