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

Preact and Relay #411

Closed
yasserkaddour opened this issue Nov 21, 2016 · 36 comments · Fixed by #2692
Closed

Preact and Relay #411

yasserkaddour opened this issue Nov 21, 2016 · 36 comments · Fixed by #2692

Comments

@yasserkaddour
Copy link

Hi,
I just tried to switch to preact on my relay project using preact-compat, it's compile but I have this error on the console.

GraphQLStoreChangeEmitter.js:82Uncaught TypeError: this._batchUpdate is not a function
at e._processBroadcasts (http://localhost:9000/assets/app.js:31:3416)
at http://localhost:9000/assets/app.js:31:3104
at i (http://localhost:9000/assets/app.js:10:19989)
at http://localhost:9000/assets/app.js:10:20732
at MutationObserver.r (http://localhost:9000/assets/app.js:13:20214)
e._processBroadcasts @ GraphQLStoreChangeEmitter.js:82
(anonymous function) @ GraphQLStoreChangeEmitter.js:63
i @ core.js:37
(anonymous function) @ core.js:123
r @ browser-raw.js:52

I think preact is awesome, I reduced my gzip bundle from 160k to 124k with it, it's just to bad it's doesn't work. Is there something you can do about it?

Thanks

@developit
Copy link
Member

Hi Yasser - sorry I didn't reply to this earlier. Do you have an example that you started from or anything? I've not used Relay (though I'm extremely interested in it) so it's a little hard to reproduce.

@yasserkaddour
Copy link
Author

Hi Jason, Thank you for considering working on Relay ;)
I added preact to the relay-starter-kit repo here https://github.com/YasserKaddour/relay-starter-kit-preact
I got the same error message as in my relay project, I hope this will help, thank you

@developit
Copy link
Member

Awesome, I'll take a look when I get a little bit of spare time!

@twhid
Copy link

twhid commented Dec 27, 2016

I poked around at this a bit and it looks like Relay (here) is relying on react-dom to have a property called unstable_batchedUpdates. This function is what is what is assigned to that property.

It would seem that preact-compat would need to implement this in order to support using preact with Relay. I'm new to both Relay and the innards of React and have no clue how to fix this, but thought the leg work in tracking it down might be helpful.

I'm also interested as it reduces a vendor bundle gzipped by almost 40% (148 -> 93k).

@sjchmiela
Copy link

Any progress on that? I think Relay compatibility would really push Preact forward, as it's a pretty common pattern to use React with Relay. :)

@developit
Copy link
Member

developit commented Jan 17, 2017

Agreed, though I've not yet had time to fix this. If someone has a few spare cycles, it seems like we could just use a shim function that immediately invokes its first argument:

function unstable_batchedUpdates(callback) {
  callback();
}

... and just export it at the bottom of the library.

@sjchmiela
Copy link

Created a pull request preactjs/preact-compat#289, unfortunately it only helped with the error, nothing renders. Any ideas?

@nickhudkins
Copy link

@sjchmiela am running into this now, it looks like it may take a little more than just calling callback(), I'm going to spend a little bit of time seeing if I can figure out how to get this to work.

https://github.com/facebook/react/blob/7dc5f91d886fcfd5bd9db40449370ce5d88ea188/src/renderers/shared/stack/reconciler/ReactUpdates.js#L94-L97

@nickhudkins
Copy link

Oh boy. Not sure what I have gotten myself into 👍

@nickhudkins
Copy link

hey @sjchmiela are you also using React Router in your project? Tracing through this, it looks like @developit was correct in suggesting that we simply need to call callback() but it gets hung up further down the render chain. Still digging as I have time.

@nickhudkins
Copy link

More importantly, @sjchmiela if you can check if "blank" means that <undefined></undefined> is being rendered to the DOM, I can at least know that my blank screen and your blank screen are likely the same issue 👍 it looks like this was a problem in the past with React Router. From what I can tell, createElement is disappearing :(

@sjchmiela
Copy link

Exactly, @nickhudkins, I use React Router and undefined is being rendered to the DOM. :) Good job!

@nickhudkins
Copy link

nickhudkins commented Jan 31, 2017 via email

@ForsakenHarmony
Copy link
Member

ForsakenHarmony commented Jan 31, 2017

@twhid actually it's this function assigned to this property afaik (because dom, but they might be functionally similar)

@nickhudkins
Copy link

@sjchmiela good news it is not react-router :) nor is it Relay. I am able to successfully render a Relay.Renderer and Relay.Container with react-router. react-relay-router appears to be where it may be getting hung up.

@nickhudkins
Copy link

nickhudkins commented Jan 31, 2017

@developit could you help me understand: https://github.com/developit/preact/blob/master/src/vdom/component-recycler.js#L18-L32

From what I can tell, the expectation is that either inst will have a property base or, inst.nextBase gets set if we can find the component in the components list.

The reason this is important, is that the failure I am seeing is as a result of buildComponentFromVNode failing at the "root render" when dom is undefined still, we hit: https://github.com/developit/preact/blob/master/src/vdom/component.js#L232 and c.base is undefined resulting in <undefined></undefined> getting rendered.

False alarm, sorry about that, though hoping your knowledge of the code may reveal what's going on here 👍

@nickhudkins
Copy link

@sjchmiela you can follow along with my debugging here: https://github.com/nickhudkins/preact-relay-playground

@developit
Copy link
Member

developit commented Feb 1, 2017

Quick investigation shows that it seems they're returning props.children, which might be an Array in some combination of preact + preact-compat. We'll need to guard against that in preact-compat somehow - there's already a place to do so. Just need to wrap render and strip arrays at the root down to their first value.

@nickhudkins
Copy link

@developit thanks for this :) :smashes head into wall: I've got some time later today so maybe I can get this wrapped up.

@nickhudkins
Copy link

@developit pardon my ignorance, but isn't that what is happening here.

@developit
Copy link
Member

Yup, not sure what setup I was thinking would skip that. I did just publish preact-compat@3.13.1, which addresses an issue where props could be undefined for Pure Functional Components in certain cases. Not sure if it's related, it should do a better job of enforcing propsHook() for those types of components.

@ticky
Copy link

ticky commented Sep 15, 2017

I just encountered this, it looks like react-router-relay is trying to render a StaticContainer with an undefined children prop, which then triggers the Children error.

I’m hoping someone understands this better than I do! :)

@sjelfull
Copy link

Hit this same issue with mobx-react :(

842:11-34 "export 'unstable_batchedUpdates' was not found in 'react-dom'

@dangerousdan
Copy link

mobx-react also breaks because of a missing unstable_batchedUpdates as of mobx-react v4.3.1.

@sjelfull try tagging your mobx-react to v4.3.0-rc1 or v.4.2.2.

@developit
Copy link
Member

Anyone interested in submitting a PR to preact-compat, adding unstable_batchedUpdates() to the exports?

@adamdickinson
Copy link

I've too have run into this same issue of relay components not rendering at all in Preact.

After some investigation, I discovered is that some Relay components (at least in relay-modern) won't return a VNode when rendered, which seems to be why things aren't being put into the DOM. It appears that the render of a RelayFragmentContainer merely resolves the required data and returns the relevant Component, when then in turn is to be rendered to give a VNode.

I've been able to hack in a fix for me that resolves that issue in the renderComponent function at preact@master:98 as follows:

        ...
	if (!skip) {
		rendered = component.render(props, state, context);
                if (rendered && rendered.render) rendered = rendered.render()  // << Added

		// context to pass to the child, can be updated via (grand-)parent component
		if (component.getChildContext) {
        ...

So it sounds like React perhaps manages recursive rendering of some sort to produce a VNode, whereas Preact renders once and assumes it gets a VNode out of it.

I might be way off here, and the issue is more than likely somewhere else, but I hope this helps! If I'm on the right track, happy to submit a PR - really want to get Relay and Preact playing nice; they're both such awesome tools.

@renatobenks-zz
Copy link

@adamdickinson is really possible to use Relay at preact?

@adamdickinson
Copy link

@renatobenks with the above patch, it runs very well. The only source of incompatibility seems to be a relatively minor difference in implementation between preact and react.

I have noted a small potential issue with component recycling, but I haven't investigated it well enough to be able to reliably replicate.

@kevlened
Copy link

kevlened commented May 16, 2018

@adamdickinson Your suggestion worked for me! I haven't noticed the recycling issue yet.

edit - it appears the additional render check is more appropriate in doRender, so it only affects components that don't already have render on their prototype.

The render called by Preact's renderComponent method calls doRender from the component-recycler. doRender is only used if the target component doesn't have render on its prototype. Most components have a render method, so modifying doRender minimizes the scope of any performance impact from the additional render check.

@developit
Copy link
Member

I'm confused what Relay is doing in the case where this fix is needed. There is a PFC that returns a component constructor - does react somehow know to instantiate that despite it not being attached to a VNode? I've never seen that behavior before...

@kevlened
Copy link

In React 16, it appears there's a check to determine the return type of a PFC in mountIndeterminateComponent

function mountIndeterminateComponent(
  current,
  workInProgress,
  renderExpirationTime,
) {
  ...
  value = fn(props, context);
  ...
  if (
    typeof value === 'object' &&
    value !== null &&
    typeof value.render === 'function' &&
    value.$$typeof === undefined
  ) {
    // Proceed under the assumption that this is a class instance
    ...
  } else {
    // Proceed under the assumption that this is a functional component
    ...
  }
}

This is within the context of Fibers though, making it harder to see why this is happening. I'll see how React 15 handles it

@kevlened
Copy link

In React 15, there's a check in mountComponent for a render method. If one isn't present, it's created.

// Initialize the public class
var doConstruct = shouldConstruct(Component); // this returns false for functions
var inst = this._constructComponent(
  doConstruct,
  publicProps,
  publicContext,
  updateQueue,
);
var renderedElement;

// Support functional components
if (!doConstruct && (inst == null || inst.render == null)) {
  renderedElement = inst;
  ...
  inst = new StatelessComponent(Component);
  this._compositeType = CompositeTypes.StatelessFunctional;
} else {
  if (isPureComponent(Component)) {
    this._compositeType = CompositeTypes.PureClass;
  } else {
    this._compositeType = CompositeTypes.ImpureClass; // Container is an ImpureClass
  }
}

I'm not sure if other libraries use this feature (a function returning a component)

@kevlened
Copy link

I may have worded my PR poorly, but Preact expects all PFCs to return VNodes, while React allows Components to be returned. buildReactRelayContainer returns the PFC ContainerConstructor, which returns an instance of a Container component rather than a VNode.

@dfleury
Copy link

dfleury commented Jun 9, 2020

Can someone say if something has changed since the release of Preact X?

@sventschui
Copy link
Member

I just gave the todo example from https://github.com/relayjs/relay-examples a try but unfortunately relay relies on react internals and thus will never work with preact afaict.

@marvinhagemeister
Copy link
Member

Looks like react-relay makes use of React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED which we didn't patch in compat before. Made a PR to patch this, see #2692 🎉

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

Successfully merging a pull request may close this issue.