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

Breaking change checked in for android plugin, JS and Java plugin do not work for android now. #438

Closed
jackychow opened this issue Apr 7, 2014 · 18 comments

Comments

@jackychow
Copy link

Hi, I believe @aogilvie checked in a breaking change to ConnectPlugin.java on Feb 03 in this change:
ee6c32d#diff-c25cdc153106f171cfa0c5fc8fdc1ffd

the condition in "execute" which handles "action.equals("init")" was removed but the JS plugin's init code is still calling cordova.exec with 'init' action. The repo for android as of this state does not seem to work because the plugin cannot initialize. Using an older version of ConnectPlugin.java seems to work but the old code apparently does not use the newer FB SDK and hence is filled with deprecated methods.

Can the changes be either rolled back or rolled forward for a permanent fix?

@jackychow jackychow mentioned this issue Apr 7, 2014
@aogilvie
Copy link
Collaborator

aogilvie commented Apr 8, 2014

@jackychow I tagged the previous stable https://github.com/phonegap/phonegap-facebook-plugin/releases/tag/0.4.2 please use that for now. This repo needs a develop branch badly...

@aogilvie
Copy link
Collaborator

aogilvie commented Apr 8, 2014

Also i'm not sure about your Android issue. The native parts all seem to work and have been tested by several developers. Can you list some more detail about the issues that you have? I'll take a look.

JS update coming ASAP. Sorry for the breaking change (hence we tagged the previous code)

@jackychow
Copy link
Author

I think the current master branch just wouldn't work on android at all. I'm speaking with reference to the "Simple" example which calls init on the CDV plugin. It will always come back as "Facebook Connect plugin fail on init" (refer to thread #343, which a number of people also reported the same issue). As I mentioned, this is due to the fact that init was not considered as a valid action in the plugin code you checked in on Feb 03 for ConnectPlugin.java. (you removed the condition, if (action.equals("init")) ). Unless there are other ways to init the plugin, I cannot see how this could have worked. Is the unit test not testing the hook between the JS code and the native code? Certainly, the native code itself might work, however, we cannot use it in Android because we cannot hook into the native code via the JS interface, and the two are just not working together. Again, if there are other ways that's supposed to init the plugin, I would appreciate some pointer to newer examples that would work.

One more thing, in the "initialize" method in the native code, ConnectPlugin.java, I think line #55 would fail because "fb_app_id " does not exist in any of the string resource files, could probably update documentation if that's intended to be user added.

Thanks,

@confile
Copy link

confile commented Apr 8, 2014

I have the same problem "Facebook Connect plugin fail on init"

@aogilvie
Copy link
Collaborator

aogilvie commented Apr 9, 2014

@jackychow The Facebook JS API has changed completely if you want to access native plugin code check the README.md. init() is not longer required. fb_app_id is created by plugman see config.xml.

The old FB js-sdk needs an update and wrap around the new JS API to native. This is ongoing. There is also a large pull request for new Graph API for Android which is slowing progress on the js-sdk changes.

@jackychow
Copy link
Author

Thanks @aogilvie. I still have a few questions, and if that's not too much to ask here.

  1. So I see that facebookConnectPlugin.js is the new JS interface you are pointing to. So it seems that the old JS scripts (cdv-plugin-fb-connect.js and even FB's own JS SDK facebook-js-sdk.js) are both no longer needed? Is this going to be the recommended use-case? skipping the use of the FB j-sdk entirely?
  2. And my understanding is that graph API does not work for android in the newer version (it seems to work in the last stable version), so I really should not use this new version until it's ready, am I correct?
  3. I think the reason that I ran into this issue is that I followed the manual installation guide which needs an update to be correct for the new version of the code. Together with the people who updated the plugin and ran into problem with code written before because it is no longer backward compatible. I understanding that this is now pretty much community-maintained, but moving forward when a huge update is needed later on, I think a dev branch is really necessary as apparently this change is not complete (graph API not working on android yet) and non-backward compatible (the JS interface needs to be completely changed). As for the documentation, it is confusing as of the current state because it contains both valid and invalid info. It is probably better for now to completely remove the manual installation portion of the README and just point readers to the last stable tag for the old-way which works for graph api in android. And a working example with the new way would also be self-explanatory for most users as well.

Again, I understand the update and maintainence is on voluntary basis and I very much appreciate the works you've done for this. That said, given that the plugin has been working before, maintaining the status quo that it continues to work is very important for people who are using this in their production code. Thanks!

@aogilvie
Copy link
Collaborator

aogilvie commented Apr 9, 2014

@jackychow

No problem. I'll try answer you Q's as best as possible.

  1. I think we are going for a major version release, which means break all the things and do them right. The main reasons for this was the CLI plugman install (yeah, this is the recommended install technique now). We rebuilt the JS to native API for it and have not updated the facebook-js-sdk.js to work with it yet. I'm working on that now.
  2. Here is the PR for Android graph API Android Graph API addition, Update iOS SDK, Android SDKs and update to README.md #443 (fresh off the press!) it's going in very soon. It would be really helpful if you can test that code! (leave an emoticon or comment on the PR please)
  3. I entirely agree. I'll update and simplify the README, so much junk in there! Also hoping to add a troubleshooting guide for common FB SDK setup issues. I think I can create branches to I'll add develop and this project can use git flow model properly ASAP. I understand, working with this terrible plugin before in production is the reason I got involved 🎉

I tagged the old code if you need that. Trying to move quickly on this, also appreciate any testing you can do on PRs. Awesome FB plugin on it's way! 🍻

@aogilvie
Copy link
Collaborator

aogilvie commented Apr 9, 2014

the old JS scripts (cdv-plugin-fb-connect.js and even FB's own JS SDK facebook-js-sdk.js) are both no longer needed?

This is my observation, I'm looking at writing native unit tests for testing if we have all the same APIs. In the meantime, can you confirm, as I expect, we can remove the old stuff??

@jackychow
Copy link
Author

Sure I'll test the PR for android API and comment on the PR. Thanks for moving forward!

I think the graph API would cover almost every operation that a user would want to perform. Here is my 5 cents on the new paradigm (using a new custom JS interface and ditch the FB SDK/CDV interface) though:

One advantage (I don't know how significant it is) for using the FB's own facebook-js.sdk.js is the ability to use the same code on both browser and phonegap (we just need to initialize differently based on detecting if we are in phonegap or not). So for developers who are trying to support platforms beyond IOS/Android, this is probably important. For instance, they might choose to host code on a webserver for users with desktop/BB/WP systems ... with this new paradigm two completely different branches of code has to be written (and maintained) - One uses the new phonegap-fb-plugin and one uses FB's JS SDK. This would appear quite messy if the two code paths thing is the only solution to this problem.

Also, using FB's JS SDK seems to have the additional benefit of saving the need to re-integrate new features of the API. Because as far as I know, the only reason we need to use native code in phonegap is login and logout because the whole cross site scripting prevention that FB requires us to put a requester IP when using the JS SDK to login. Once we are logged in, we can use FB's JS apis as-is. So hypothetically, FB revamp graph API a few months later and added something called X-API, then we would need to re-integrate X-API and expose that on the custom JS plugin script ... whereas, when FB release new SDK, they necessarily would have written a working X-API method in a distributed JS SDK.

That said, using FB's distributed JS SDK has it's only problem too .... somebody need to update the JS SDK code regularly as the whole cordova nativeinterface patch has to be hacked in for this to work.

@jackychow
Copy link
Author

by the way, forgive me for being dumb ... how do I get the code that you changed from the PR? I see the diffs but is there a branch that I can pull the changed files to test?
Thanks

@jackychow
Copy link
Author

Hi Ally, been away for a few days and just started picking up testing your new JS plugin.

apparently this
var exec = require("cordova/exec");

is giving me an unreference error for require.

Do I need nodejs or commonjs to make this work?

@briceicle
Copy link

@aogilvie Could you please update the README with the new setup guide?

@aogilvie
Copy link
Collaborator

#462

@bsnkengs Latest README: https://github.com/aogilvie/phonegap-facebook-plugin/blob/backwardCompatUpdate/README.md

@jackychow no need for node.js or common.js, but you must install with CLI.

@Pyo25
Copy link

Pyo25 commented May 10, 2014

I read all the discussion and I tried different solutions but I still have the "Facebook Connect plugin fail on init".
(When I retry and click on the Login button - from the Simple example - the error show is: "Cordova Facebook Connect plugin fail on login!Class not found")

Now, I'm trying to use the new JS:

  1. I removed the cdv-plugin-fb-connect.js and facebook-js-sdk.js from my index.html page
  2. I inserted the facebookConnectPlugin.js

Now, how can I instantiate the facebookConnectPlugin object ?

I tried:
var facebookConnectPlugin = cordova.require('FacebookConnectPlugin');
But the module is not found.

@kaansoral
Copy link

@Pyo25 If you are using the "cordova add" method, the JS libraries are loaded dynamically

You can just reference the library, like: facebookConnectPlugin.login etc. You don't have to import anything

@aogilvie
Copy link
Collaborator

@jackychow This issue is now fixed in develop branch. APIs should all be linked for JS, iOS and Android. I have also updated the Facebook JS SDK.

@aogilvie
Copy link
Collaborator

aogilvie commented Jun 9, 2014

@jackychow Can you check develop branch and tell me if this solves your problem? I'd like to try resolve this github issue.

@aogilvie
Copy link
Collaborator

aogilvie commented Jul 8, 2014

I'm closing this issue. @jackychow re-open it if you have difficulty with the new API.

@aogilvie aogilvie closed this as completed Jul 8, 2014
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

No branches or pull requests

7 participants
@confile @kaansoral @aogilvie @briceicle @jackychow @Pyo25 and others