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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Version Packages #174

Merged
merged 1 commit into from Oct 10, 2022
Merged

Version Packages #174

merged 1 commit into from Oct 10, 2022

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Oct 10, 2022

This PR was opened by the Changesets release GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated.

Releases

miniplex-react@1.0.1

Patch Changes

  • a43c734: Fixed: When <Component> re-renders, it is expected to reactively update the component's data to the value of its data prop, or the ref of its React child. It has so far been doing that by removing and re-adding the entire component, which had the side-effect of making the entity disappear from and then reappear in archetypes indexing that component. This has now been fixed.

    The component will only be added and removed once (at the beginning and the end of the React component's lifetime, respectively); in re-renders during its lifetime, the data will simply be updated directly when a change is detected. This allows you to connect a <Component> to the usual reactive mechanisms in React.

@hmans
Copy link
Owner

hmans commented Oct 10, 2022

Canary release for testing: miniplex-react@0.0.0-canary-20221010085529

I'm going to use that version in my project for a few hours before hitting the publish button.

cc @Madou

@itsdouges
Copy link

Cheers will give it a go now. FYI it looks like you're publishing the canary to latest tag which is probably not what you want.

image

@itsdouges
Copy link

itsdouges commented Oct 10, 2022

🤔 I had this code which worked previously but now... doesn't.

<Component name="sceneObject">
  <group ref={group} name="camera-target">
    {children}
  </group>
</Component>

Refs not being picked up anymore? I'll debug.

Hypothesizing that the strict mode change which conditionally renders the Component children broke something https://github.com/hmans/miniplex/pull/175/files#diff-5c286f5c2f9060eb93c279dc05386852049b771b7907626e6a39d68b09d3ca85R85 - I believe my code relies on it being there for the initial render but it's now not there until the second one.

@itsdouges itsdouges mentioned this pull request Oct 10, 2022
@hmans
Copy link
Owner

hmans commented Oct 10, 2022

Cheers will give it a go now. FYI it looks like you're publishing the canary to latest tag which is probably not what you want.

Argh, that was indeed not my intention. Note sure what happened there, but I'll keep an eye out the next time. Thanks for letting me know.

I had this code which worked previously but now... doesn't.

I'll look into it. Refs capturing appeared to work fine in Space Rage.

@hmans
Copy link
Owner

hmans commented Oct 10, 2022

I've managed to reproduce this. It looks like the ref is now only captured during re-renders, but not on the initial render. I'm looking into this.

@itsdouges
Copy link

itsdouges commented Oct 10, 2022

No worries, if we get context and children available for the first render it should be fixed. Currently it's conditionally rendering based on the miniplex property being available in the entity object, which it will only exist after the first render.

@hmans
Copy link
Owner

hmans commented Oct 10, 2022

Correction, the ref capturing is working fine. The components themselves don't seem to be added correctly. Looking into it further.

@hmans
Copy link
Owner

hmans commented Oct 10, 2022

No worries, if we get context and children available for the first render it should be fixed. Currently it's conditionally rendering based on the miniplex property being available in the entity object, which it will only exist after the first render.

The re-render unfortunately is necessary because the actual creation and destruction of the entity needs to happen in a layout effect, and if the children contain a <Component>, it would interact with an object that isn't a registered entity yet, and throw an error. (Which is something I'm beginning to dislike, but I don't want to make changes this big right now.)

@itsdouges
Copy link

True. Perhaps revert the strict mode fix for now?

@hmans
Copy link
Owner

hmans commented Oct 10, 2022

True. Perhaps revert the strict mode fix for now?

I'm not sure if this is related to the fix itself, or just a weird behavior that only occurs in Strict Mode (which I hadn't seen so far because I wasn't using it properly, ahem.) I'm investigating.

Fact is that the canary version works great in Space Rage, which is a much more complex project than the little test bed I'm working with at the moment. I want to understand (and fix) the issue, so I'll stay on it for a bit.

@hmans
Copy link
Owner

hmans commented Oct 10, 2022

So, yeah, I'm seeing the same issue with both fixes reverted. It must be something else entirely.

@itsdouges
Copy link

itsdouges commented Oct 10, 2022

To clarify what issue are you seeing? For me it's the change with the strict mode deferring rendering children until the second render now (when previously it would be rendered on the initial render).

This code is ran to collect a child ref for the camera target in a layout effect, previously it would find a target now it doesn't. 🤔

Simplified it looks like:

const findTargetVector = (objs: Object3D[]): Object3D => {
  for (let i = 0; i < objs.length; i++) {
    const obj = objs[i];
    if (obj.name === 'camera-target') {
      return obj;
    } else {
      return findTargetVector(obj.children);
    }
  }

  throw new Error('invariant: no marked target found');
};

function CameraTarget({
  children,
}) {
  const ref = useRef<Group>(null!);

  useLayoutEffect(() => {
    findTargetVector(ref.current.children);
  }, []);

  return <group ref={ref}>{children}</group>;
}

<CameraTarget>
  <Entity>
    <Component name="sceneObject">
      <group ref={group} name="camera-target">
        {children}
      </group>
    </Component>
  </Entity>
<CameraTarget>

@hmans
Copy link
Owner

hmans commented Oct 10, 2022

Yup, that's the one I'm trying to fix. I'm trying to get rid of that rerender, but I'm also trying to avoid the typical "bypass React's double effects" tricks if possible.

@hmans
Copy link
Owner

hmans commented Oct 10, 2022

I'm afraid this is a bigger issue because even if I change <Entity> back to directly creating the entity on render (instead of in an layouteffect), components will still be added and removed in layouteffects. I need to think about this a little longer.

@hmans
Copy link
Owner

hmans commented Oct 10, 2022

@Madou I've reverted the Strict Mode fix for now and will release 1.0.1 with the other fix so we have at least that. I think fixing Strict Mode will require some bigger changes (also to the core package, unfortunately).

@hmans hmans merged commit f4c4adc into main Oct 10, 2022
@hmans hmans deleted the changeset-release/main branch October 10, 2022 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants