clayallsopp / react.backbone

Plugin for React to make Backbone migration easier
MIT License
839 stars 60 forks source link

Does this work with React 0.11.1? #26

Closed alanhogan closed 10 years ago

alanhogan commented 10 years ago

I had some issues trying to use this with React 0.11.1, but I’m not sure if the problem was me or a version mismatch.

mountComponent is complaining it can't find this.props when I use createBackboneClass. (Note also that React.createBackboneClass no longer seems to be defined; I had to import react.backbone as RB and then call RB.createBackboneClass.

screen shot 2014-07-28 at 4 37 26 pm

clayallsopp commented 10 years ago

Hm, I ran our tests against React 0.11 and they pass

Did you try running your project against React 0.10 to verify that 0.11 causes the issue? There were also some breaking changes in that release, which might also be affecting your project

alanhogan commented 10 years ago

Right, thanks, Clay. I previously looked at breaking changes and didn’t see any obvious culprits. I am trying the most basic usage of React.Bacebone possible. I am starting from the coffee-react-quickstart project, however.

Anyway, if you feel generous, check out this branch (then run npm install && cult watch and browse to localhost:8080) to see the problem.

alanhogan commented 10 years ago

If I try React.createBackboneClass, the error message is as follows:

Uncaught TypeError: undefined is not a function bundle.js:924
replaceCreateClass bundle.js:924
(anonymous function) bundle.js:900
__webpack_require__ bundle.js:400
fn bundle.js:58
replaceCreateClass bundle.js:696
SUCCESS_LEVELS bundle.js:534
__webpack_require__ bundle.js:400
fn bundle.js:58
(anonymous function) bundle.js:429
__webpack_require__ bundle.js:400
fn bundle.js:58
(anonymous function) bundle.js:420
(anonymous function)
alanhogan commented 10 years ago

Hey, you know what, it looks like it has to do with the hotloader that comes with the quickstart.

alanhogan commented 10 years ago

Thoughts on how to resolve this are welcome

alanhogan commented 10 years ago

But it’s clear at this point the bug does not lie with react.backbone

alanhogan commented 10 years ago

Update: the hotloader does not seem to be to blame, after all. See this update

alanhogan commented 10 years ago

Think there may be some sort of require-order issue here?

alanhogan commented 10 years ago

See also: rackt/react-router#146

markijbema commented 10 years ago

In that issue it's suggested the dependencies aren't specified the right way. Personally, I use this plugin via bower, using the rails asset pipeline. If any of the other dependency management systems is having a problem, I'd be happy to merge a pull-request, but I can't test/verify these errors myself (and given the order of pr's, I suspect @clayallsopp isn't using AMD/... either).

Imo it's really sad how this problem is currently solved in JS-land, you have to support multiple systems, but of course you're not using them (all) yourself. I would be very happy if at some point we get one system everyone uses :)

gaearon commented 10 years ago

I would be very happy if at some point we get one system everyone uses :)

There's a "help everyone converge on NPM" movement.

In this case, it looks like a rough edge of NPM: your package specifies react and backbone as dependencies but it means that if client code specifies different versions of them, NPM will try its best to please everyone and give your library its own react/backbone. This makes sense for Node.js (different versions of the same package can be safely used by some of your dependencies) but rarely makes sense for the browser.

You should (probably) move react and backbone to peerDependencies since you require the client code to have React/Backbone, but don't ever want your lib to have a private copy of them.

clayallsopp commented 10 years ago

I've attempted to make the change in 209cb8db32600021318f925bdce3db428a84e30c - can someone try that out? (ie use "react.backbone": "git://github.com/usepropeller/react.backbone#master" in your dependencies) (but tbh I'm sort of out of my element wrt npm packaging etc)

alanhogan commented 10 years ago

Thanks, @clayallsopp! That absolutely has solved the problem! Big thanks for pointing out the exact problem and solution, @gaearon!

gaearon commented 10 years ago

No problem, just found the same problem with Underscore in my project, unnoticed for weeks. :-)

alanhogan commented 10 years ago

Maybe Webpack can start warning about this

alanhogan commented 10 years ago

Filed as webpack/webpack#385 if you'd like to chime in.