search

facebookarchive / react-meteor

React rendering for Meteor apps
948 stars 114 forks source link

Fix loading issue in Cordova apps. #74

Closed ukabu closed 9 years ago

ukabu commented 9 years ago

Fix #54.

Now properly export the React object on the client.

This PR removes the inject-react.js script. It is replaced by client-react.js that just export the React object and removes it from window (as per Meteor recommendation for integrating existing libraries).

It also directly add the non-minified react-with-addons-...js and rely on building for production to have Meteor minify it.

facebook-github-bot commented 9 years ago

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

facebook-github-bot commented 9 years ago

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

ukabu commented 9 years ago

Please hold-on on merging this... Seems I'll have to add another commit due to some issues when running inside Cordova. Server and Client are fine.

Forget this comment, for some reason my Cordova app wasn't updated on my device. It works fine.

idris commented 9 years ago

:+1:

ukabu commented 9 years ago

It's been more than two weeks since this PR was created, any idea if and when it is going to be merged?

iSuslov commented 9 years ago

Hey guys! Could you please merge this ASAP?

idris commented 9 years ago

@benjamn is anyone paying attention to this repo? If not, maybe it should be handed over to someone in the community?

ahoereth commented 9 years ago

+1 for this! There is another community meteor-react package which handles the 'use minified version' step well, see here.

ukabu commented 9 years ago

@ahoereth Meteor doesn't have a good way to know if the target build is for production. Their hack may seem to work but is probably a bit brittle and will not cover all cases that the app is run in production mode (ex: does meteor run --production spawn a meteor build or does it just run the build in the same process, which would make their hack to not use the minified version).

This is why I did not include any such hack in my PR. We will still get warnings and some other sanity check from the unminified react, but I prefer to have something that is working and is predictable even if there is a slight performance hit.

In any case, I'm staring to be impatient to see this PR merged.

thanhlvbk commented 9 years ago

+1

particle4dev commented 9 years ago

+1

benjamn commented 9 years ago

I'm sorry this PR has gone unaddressed for so long.

The concern I have about this pull request is that the minified version of the file provided by the Facebook React team is different from what Meteor will produce when it minifies the un-minified file. In particular, the Facebook-provided version strips the string messages from all invariant calls, which saves a ton of bytes. In other words, @ukabu's comment is unfortunately right (see above).

I realize the Inject.rawHead call is not pretty, but it's the only way I know to inject different versions of the React library in development and production. Though the inject-react.js file is evaluated only on the server, it adds a <script> tag to all pages displayed by the client, so it has the effect of evaluating the React library on the client.

I believe the problem reported in #54 is a symptom of Inject.rawHead not working properly in Cordova apps. I'm going to look into fixing that, and if I can't fix it that way, I'll be happy to merge this pull request.

ukabu commented 9 years ago

Thanks @benjamn for looking into this.

I don't think there is any way to make Inject.rawHead working in Cordova as it is code that runs on the server. The cordova app loads it's first HTML page locally, if the React variable is not defined, it will just fail. Even if the script is injected later, the UI will not be rerendered and we will still have a blank screen.

Note that the PR also fix the issue of the React variable not being exported properly (Meteor way) on the client. Before this, meteor packages that would depend on this Meteor package would not have access the React variable as it would be defined after the package code is run. Exporting the variable properly allow the use of Package['reactjs:react'].React.

Without exporting React properly, creating Meteor package of React component would not be feasible.

I don't think there will be any clean/perfect solution to this issue as long as Meteor does not provide some way to know if we run in production or development mode. Or if we could provide the unminified and minified version to addFiles so it would select the one it wants automagically.

benjamn commented 9 years ago

Ok, I'm pretty eager to get rid of Inject.rawHead anyway, and this is a good excuse. Hopefully when we find a way of generating different builds for dev/prod, this will all become a lot easier.