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

TransitionGroup addon causes "Only a ReactOwner can have refs" #204

Closed
frol opened this issue Nov 5, 2016 · 20 comments
Closed

TransitionGroup addon causes "Only a ReactOwner can have refs" #204

frol opened this issue Nov 5, 2016 · 20 comments

Comments

@frol
Copy link
Contributor

frol commented Nov 5, 2016

With a help of @mlaursen (mlaursen/react-md#150), I minimized the reproduction of the bug we encounter in #119 and #196.

Here is the pages/index.js:

import React from 'react'
import TransitionGroup from 'react-addons-transition-group'

class Test extends React.Component {
  render() {
    return <div ref={woop => console.log(woop)} />;
  }
}

export default class App extends React.Component {
  render() {
    return <div>
      <TransitionGroup>
        <Test key="test-1" ref={test => console.log(test)} />
      </TransitionGroup>
    </div>
  }
}

Once you open the page, you will get the following in your browser console:

Warning: getDefaultProps is only used on classic React.createClass definitions. Use a static property named `defaultProps` instead.
Warning: getInitialState was defined on ReactTransitionGroup, a plain JavaScript class. This is only supported for classes created using React.createClass. Did you mean to define a state property instead?
<div data-reactid="3"></div>
Uncaught Error: addComponentAsRefTo(...): Only a ReactOwner can have refs. You might be adding a ref to a component that was not created inside a component's `render` method, or you have multiple copies of React loaded

If I drop the TransitionGroup component, the page works fine.

Does anyone have a clue of what is the cause of this issue?

@frol
Copy link
Contributor Author

frol commented Nov 5, 2016

This seems to be the issue in react-addons-transition-group package handling: facebook/react#7874

@frol
Copy link
Contributor Author

frol commented Nov 5, 2016

Inspecting the HTML response from Next.js server, I see that ReactJS gets bundled inside the __NEXT_DATA__, while it is also bundled in next-dev.bundle.js, so it ends up with two copies of ReactJS on a page.

@frol
Copy link
Contributor Author

frol commented Nov 5, 2016

Given the fact that react-addons require private ReactJS methods access (i.e. they require to be bundled together with ReactJS), it seems that Next.js should consider one of the following approaches:

  • Don't bundle ReactJS into Next.js, and provide developers a choice of using either "pure" ReactJS or ReactJS-with-addons
  • Provide extra Next.js bundles for ReactJS-with-addons (Next.js will end up with 4 bundles: dev/prod * ReactJS-with-addonds/ReactJS-without-addons)

/cc @nkzawa

@nkzawa
Copy link
Contributor

nkzawa commented Nov 5, 2016

Don't bundle ReactJS into Next.js, and provide developers a choice of using either "pure" ReactJS or ReactJS-with-addons

Actually, we are considering if should do this.
It allows users to use any versions of React and make things more explicit.

@nkzawa nkzawa added bug labels Nov 5, 2016
@frol
Copy link
Contributor Author

frol commented Nov 5, 2016

@nkzawa Do you have it on your roadmap? It is quite critical to me as I was going to start a new project as soon as possible, and I would like to use UI toolkits, which heavily use the React Animation add-ons...

If it is not on the roadmap, could you, please, guide me on how I can build a custom Next.js bundles with ReactJS-with-addons?

@nkzawa
Copy link
Contributor

nkzawa commented Nov 6, 2016

Unfortunately, it's not on our roadmap yet, but It seems critical to me. cc @rauchg

If it is not on the roadmap, could you, please, guide me on how I can build a custom Next.js bundles with ReactJS-with-addons?

You have to add import 'react-addons-transition-group' to client/next.js and run gulp release.
But you'd have to fix the webpack config in next.js too.

@frol
Copy link
Contributor Author

frol commented Nov 6, 2016

I have succeeded in patching Next.js, so it uses external React, ReactDOM, and ReactMD (Metarial UI toolkit)! I will try to clean my patch up and will publish it as a PR, though I am not a JS developer, so I don't expect it will get merged, but at least, it can be a starting point for someone.

@sedubois
Copy link
Contributor

sedubois commented Nov 8, 2016

We're seeing the same kind of warnings due to React being included twice when using Auth0-lock: https://github.com/luisrudge/next.js-auth0/issues/3

@frol
Copy link
Contributor Author

frol commented Nov 8, 2016

@sedubois You may try using the dist as a drop-in replacement for ./node_modules/next/dist from my #221 to see if that helps.

@xcadaverx
Copy link

xcadaverx commented Dec 6, 2016

@frol I'm getting these same errors using next.js with both react-images library and the react-photo-gallery. I've tried replacing all instances of react-md and var ReactMD in your #221 dist.tar.gz to react-photo-gallery/var ReactPG, but still receive the same error. Could be the part that referenced a link to react-md on unpkg, as i wasnt exactly sure which react-photo-gallery JS file to reference there, but went with https://github.com/neptunian/react-photo-gallery/blob/master/lib/Gallery.js.

Any help appreciated. Cheers!

@rauchg
Copy link
Member

rauchg commented Dec 7, 2016

Agreed that we should fix this @nkzawa

@arunoda
Copy link
Contributor

arunoda commented Dec 15, 2016

It's hard to manage few different types of bundles. So, I think we could add following addons bundled with Next by default.

  • react-addons-css-transition-group
  • react-addons-transition-group
  • react-addons-create-fragment

@rauchg
Copy link
Member

rauchg commented Dec 15, 2016

I think it should be straightforward to make them work. Just add them to dependencies, maybe extend webpack somehow and voila. I don't want to bloat next.

@arunoda
Copy link
Contributor

arunoda commented Dec 15, 2016

@rauchg I agree.
I wanna find the root cause as well.
If not this should be the way. (I assume these addons doesn't bloat next)

@rauchg
Copy link
Member

rauchg commented Dec 16, 2016

A whitelist of addons in my mind is bloatware.

@xcadaverx
Copy link

xcadaverx commented Dec 16, 2016 via email

@revolunet
Copy link

FYI this module works as a replacement for basic ReactCSSTransitionGroup stuff that doesnt work (yet) in Next.js : http://react-component.github.io/animate

arunoda added a commit to arunoda/next.js that referenced this issue Dec 16, 2016
React addons require React in a special way.
That causes Webpack to push React into the app's bundle.
This fix adds new externals entries to prevent that.
@arunoda arunoda mentioned this issue Dec 16, 2016
@arunoda
Copy link
Contributor

arunoda commented Dec 16, 2016

@rauchg I got it. I mean that's last option.
But I found the root cause and here's the fix: #401

@ajihyf
Copy link

ajihyf commented Dec 16, 2016

How about using webpack DllPlugin to bundle react and other vendors?
https://github.com/webpack/docs/wiki/list-of-plugins#dllplugin

rauchg pushed a commit that referenced this issue Dec 16, 2016
React addons require React in a special way.
That causes Webpack to push React into the app's bundle.
This fix adds new externals entries to prevent that.
@revolunet
Copy link

thanks @arunoda 👍 ReactCSSTG works now flawlessy :)

@lock lock bot locked as resolved and limited conversation to collaborators May 12, 2018
ForsakenHarmony pushed a commit that referenced this issue Jul 25, 2024
This is a very early version of the next-dev test runner. I'm opening this early to get thoughts from folks re: the direction of the design and implementation.

Fixes #204 

Currently it:
* Discovers integration test fixtures from the filesystem. Right now these are expected to be single files that get bundled and will eventually include assertions. This is powered by the test-generator crate, which allows us not to have to manually enumerate each case. We could consider using this for the node-file-trace tests as well.
* Starts the dev server on a free port and opens a headless browser to its root. The browser control is implemented with the https://crates.io/crates/chromiumoxide crate, which expects Chrome or Chromium to already be available.

Eventually it will:
* [x] Implement a minimal test environment loaded in the browser so that assertions can be run there from bundled code.
* [x] Report back the results of these assertions to rust, where we can pass/fail cargo tests with those results.

In the future it could:
* Possibly include snapshot-style tests to assert on transformed results. This could be in the form of fixture directories instead of files cc @jridgewell
* Support expressing special configuration of turbopack in a fixture, possibly as another file in the fixture directory.
* [x] ~Possibly support distributing tests to a pool of open browsers instead of opening and closing for each test.~

Test Plan: See next PRs
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants