My ArcherElements are defined in another component, so I am getting "Could not find "unregisterChild" in the context of <ArcherElement>" about react-archer HOT 20 CLOSED

@AncientSwordRage Context is not passed through into your render component, and React has no smart way to make that possible. The container could expose the context and you could manually pass it, but I don't think that just works off the top of my head.

from react-archer.

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?
It is indeed exported but only for the tests of the library.

@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 🧐

from react-archer.

@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 withContainerContext API looks good. You can just export that itself as part of the public API, so you don't have to pass refs. This is all valid JS.

from react-archer.

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

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.

No, they weren't.

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.

You seem to be the only one that is confused.

from react-archer.

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

from react-archer.

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 think it should be working properly with direct children (otherwise there'd be another issue I guess), but here my guess is that the callback-style rendering function might raise an issue. I really have no idea for now, it's only a random guess.

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 😊
(look at ArcherContainer and ArcherElement)

from react-archer.

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?

from react-archer.

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?

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.

from react-archer.

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.

from react-archer.

Thank you for the answers @jfo84. This is super helpful 😊

@pierpo The withContainerContext API looks good. You can just export that itself as part of the public API, so you don't have to pass refs. This is all valid JS.

What do you mean I can export that as part of the public API?
Since it has to be an instance method of the ArcherContainer (it references other methods and properties), it can't be accessed but by the ref, right? 🤔

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.

What do you mean by "child function API for the container"? What would the API look like for @AncientSwordRage's usage then ?

from react-archer.

@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 createStore pattern. This opens up the API by giving you the tools to essentially create a context, just like this. But do we really want this kind of complexity? Let's just make it a singleton and it's all much simpler. Otherwise we're going to have a lot of difficulty handling this kind of thing as an instance concept. If it becomes needed, we can always add it.

from react-archer.

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.

from react-archer.

@pierpo Something like this would work

<MyApp>
    <ArcherContainer ref={this.storeContainerRef}>
        {({ ArcherContext }) => (
            <ThirdPartyComponent
                itemRenderer={() => (
                    <ArcherContext.Provider>
                        <ContainsArcherElement />
                    </ArcherContext.Provider>
                )}
            />
        )}
    </ArcherContainer>
</MyApp>

from react-archer.

The following ticket mentions a refactored version of react-archer, and changes got made do to the current context initialization approach.

#99

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.

export const ArcherContainer: FC<ArcherContainerProps> = ({
  children,
  ...rest
}) => {
  return (
    <RefProvider>
      <TransitionProvider>
        <ArcherSurface {...rest}>{children}</ArcherSurface>
      </TransitionProvider>
    </RefProvider>
  );
};

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.

from react-archer.

@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 React.Children.only. This API is awful and breaks all sorts of things that it shouldn't have any business breaking. The core team should just remove the leaky abstraction that is React.Children.

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.

from react-archer.

@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:

• split things into smaller pieces,
• reduce state in archer container, because when it`s one thing, I learned in the past, several and probably huge state slices in a single component introduce side effects, which are complicated to track and handle when they arise.
• smaller parts also simplify testing, which is not significant in this project.

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.

from react-archer.

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.

@pierpo Something like this would work

<MyApp>
    <ArcherContainer ref={this.storeContainerRef}>
        {({ ArcherContext }) => (
            <ThirdPartyComponent
                itemRenderer={() => (
                    <ArcherContext.Provider>
                        <ContainsArcherElement />
                    </ArcherContext.Provider>
                )}
            />
        )}
    </ArcherContainer>
</MyApp>

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?

from react-archer.

This should be solved by @jfo84's PR #101
Thank you for your amazing job with this @jfo84!

from react-archer.

@pierpo Thanks Pierre. Looking forward to working together in the future :)

from react-archer.

@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 😉

from react-archer.