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
My ArcherElements are defined in another component, so I am getting "Could not find "unregisterChild" in the context of <ArcherElement>" #100
Comments
Hello! I was not aware of this problem. Definitely a bug! I don't have a good knowledge of the React Context, I don't know how it behaves in such cases. I won't look into it in the short term because there's a very important issue that I'd like to focus on, but I'll have a look after that. You could also have a look yourself, the code base is quite small 😊 |
@AncientSwordRage Context is not passed through into your |
I had a quick look at the source and it seems as though the context is exported and reimported. I'll need to double check but I don't see why that would impact this use case? |
Where is it exported/reimported? @jfo84 is right, the only way I can think of is indeed exposing the context, but I have no idea how it would work out. This: render() {
const SvgArrows = this._computeArrows();
return (
<ArcherContainerContextProvider
value={{
registerTransitions: this._registerTransitions,
unregisterTransitions: this._unregisterTransitions,
registerChild: this._registerChild,
unregisterChild: this._unregisterChild,
}}
>
<div style={{ ...this.props.style, position: 'relative' }} className={this.props.className}>
<svg style={this._svgContainerStyle()}>
<defs>{this._generateAllArrowMarkers()}</defs>
{SvgArrows}
</svg>
<div style={{ height: '100%' }} ref={this._storeParent}>
{this.props.children}
</div>
</div>
</ArcherContainerContextProvider>
);
} should become... this? withContainerContext = (children: React$Node) => {
return (
<ArcherContainerContextProvider
value={{
registerTransitions: this._registerTransitions,
unregisterTransitions: this._unregisterTransitions,
registerChild: this._registerChild,
unregisterChild: this._unregisterChild,
}}
>
{children}
</ArcherContainerContextProvider>
);
};
render() {
const SvgArrows = this._computeArrows();
return this.withContainerContext(
<div style={{ ...this.props.style, position: 'relative' }} className={this.props.className}>
<svg style={this._svgContainerStyle()}>
<defs>{this._generateAllArrowMarkers()}</defs>
{SvgArrows}
</svg>
<div style={{ height: '100%' }} ref={this._storeParent}>
{this.props.children}
</div>
</div>,
);
} so that you can do <MyApp>
<ArcherContainer ref={this.storeContainerRef}>
{/* do a cleaner access with null checks, this is just a quick example */}
<ThirdPartyComponent itemRenderer={() => (this.containerRef.current.withContainerContext(<ContainsArcherElement />))}
</ArcherContainer>
</MyApp> I think this would work 🤔 I haven't tried it out, maybe I'm wrong. But I'm not sure what the impact of the change above would have on ArcherContainer in general 🧐 |
Exported here https://github.com/pierpo/react-archer/blob/master/src/ArcherContainer.js#L81 Imported here https://github.com/pierpo/react-archer/blob/master/src/ArcherElement.js#L6 I will give the ref passing a go, and see if I can make that work. EDIT: Looks like the code snippets alone were not enough for me. I'm not very familiar with the class syntax, but I will try and see if I can make changes this weekend/next week, to make this work. |
@AncientSwordRage The importing/exporting is correct. This is going to sound obnoxious but I did talk to Dan Abramov about this API design before hooks came out, and I was exporting the context consumer like this in all the examples I gave. @pierpo The |
You could also be clever with a child function API for the container. You typecheck and if the child is a function, you pass the context as an arg. Would be very clean but maybe a bit over the top. Your solution is simple and works. |
Thank you for the answers @jfo84. This is super helpful 😊
What do you mean I can export that as part of the public API?
What do you mean by "child function API for the container"? What would the API look like for @AncientSwordRage's usage then ? |
@pierpo It essentially functions as a singleton, right? I am not sure why you would want multiple containers, although it could make sense in some use cases. This is why Redux uses the |
To be clear, the context is currently implemented as a singleton. The context is created on initialization of the component file, not during component invocation. |
@pierpo Something like this would work <MyApp>
<ArcherContainer ref={this.storeContainerRef}>
{({ ArcherContext }) => (
<ThirdPartyComponent
itemRenderer={() => (
<ArcherContext.Provider>
<ContainsArcherElement />
</ArcherContext.Provider>
)}
/>
)}
</ArcherContainer>
</MyApp> |
The following ticket mentions a refactored version of react-archer, and changes got made do to the current context initialization approach. In the current version context, init gets made when ArcherContainer starts. I made the following change. ArcherContainer does context init, both for tracked refs and transitions.
A new component was introduced, called ArcherSurface, which carries the code of archer container in a refactored version. project-millipede/millipede-docs@ac8fd2b When this is relevant, please let me know. I promised some contributions to create a pull request from those changes last week. |
@gurkerl83 Hey - I have a lot of thoughts here so I'll try to run through them all. The first thing is that it's a huge piece of work, and IMO it would have to be split up into different pieces. I have not paid much attention to the project recently, and even still I noticed the break and made my opinions clear about the v2.0 changes: that they were aggressive and breaking and would cause issues, especially the use of So, in the sense that your work is fixing bugs, I think that the bugs shouldn't have existed in the first place. More honestly, this is an internal rewrite. I did also rip out most of the internals in this project for v1.0, so I am maybe biased towards my own work, but I question how useful it is. The context refactor itself is what I have personally argued for in the past, especially the separation of the contexts into a state and implementation (dispatch) context. However, as I have spent time away from React, I question the idea entirely. I found multiple times in private settings that these new context approaches are, in fact, less performant and more buggy than using simple tools like state and Redux. For all the flack that Redux gets, it is absurdly performant, and it is built to be very lazy. Context is not, at all. The myriad use of providers also bloats the VDOM, so for all the optimizations you are getting for removing classes in favor of functions, it is questionable if it helps at all. If you want it to be performant, you have to build the optimizations yourself. Taken to its logical extent, you get big options hashes like the ones from Redux connect. And of course, if you build it all yourself, your surface for bugs skyrockets. In closing, what does this give the project? I have also championed moves from flow to TypeScript, and again, it is almost the same thing. Flow is more aggressive about interpolating types, so if coverage isn't great, you actually have a better tool at your disposal. That's not an issue here: what I'm trying to do is poke holes in this idea that progress for progress's sake is good. New does not mean better. If this work makes the project simpler, faster, or provides a more extensive API; then I am 100% onboard. If not, then eh. |
@jfo84 It is an internal rewrite, with a few changes here and there, but with no additional features added. It was a result based on the breaking changes introduced, which was not working using custom components for archer element children. The authors are sorry that changes like the use of Children.only were built-in, but that was not a breaking change at all, the reasons are different when reading through the ticket #99. After looking at the code for the first time, I realized there is plenty of room for improvement. Reasons for the rewrite:
Further, you mentioned Redux, which itself is entirely built on-top of Reacts context API, at least in the newer versions. In the third paragraph of your comments, you mentioned redux a lot. Please do not confuse the reducers parts of my rewrite with redux. There is no use of Redux at all. Reducers are a pretty new React feature. Why I choose a functional approach instead of a class-based approach? Not because of stylistic things, I hold back to classes in my projects as long as I can, but new components I implement most of the times as functional components, Why? A primary reason, to access hooks and to enforce me to think in smaller pieces. With my contribution here and there in this project, I only give options, you guys as the contributors have to decide which way to go. |
@gurkerl83 Don't talk down to me. If you actually knew what you were talking about, then you would know that React Redux is not built with context. I know that because Mark asked me to add my input on the v6.0 release, where they moved from subscriptions to context. That was a disaster, as it added back the old zombie children issues with legacy context, and they eventually removed it.
No, they weren't.
You seem to be the only one that is confused. |
Regardless of any confusion, on anyone's part. I feel like using functional components and hooks would potentially simplify things. (I will openly admit that I am biased, in that the majority of work I've done with React is with functional classes, and I am no a fan of typescript). I'm not convinced a rewrite is needed to fix this bug, so it might be more productive to move this conversation out of the ticket.
I tried the above code, with my own fork of react-archer (with the changes above), and I couldn't get it to work. Is this something simple to add now or this not enough? |
@AncientSwordRage Sorry, but I can only be so nice sometimes. When someone tells you that you're confused and an idiot, and then goes on to reference a pull request that you contributed to as evidence, I'm going to tell them off. Engineers and their egos know no bounds. The feature has to be added, but I can look into it later today. |
@pierpo Thanks Pierre. Looking forward to working together in the future :) |
I'd love to 😊 Thanks for your super helpful contribution! I hope we'll get more 😉 |
I have a component that looks a little something like:
And then
ContainsArcherElement
is similar to:But I'm getting the full
Could not find "unregisterChild" in the context of <ArcherElement>. Wrap the component in a <ArcherContainer>.
but it should be wrapped, just not immediately.Is there a way around this, and is it a bug or a feature?
The text was updated successfully, but these errors were encountered: