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

add options to not return the raw data and unpaginate profile fields #25

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

chkuendig
Copy link

Hi,
for some reason GitHub put together both commits in one PR:

  • In some circumstances returning the raw data isn't necessary and might even be a waste of resources (e.g. when the profile will be saved to a database). This PR ads an option to disable this behavior (default behavior isn't changed)
  • For some fields Facebook doesn't return all values but paginates them (see https://developers.facebook.com/docs/graph-api/using-graph-api/v2.2#paging ). With this PR its possible to specificy these fields with the option "unpaginateFields" and they will be completely loaded. Obviously this will result in a performance hit so it should only be used when absolutely necessary (default behavior isn't changed)

Cheers

  • Chris

@chkuendig chkuendig changed the title add option to not return the raw data received from facebook add options to not return the raw data and unpaginate profile fields Mar 23, 2015
@drudge
Copy link
Owner

drudge commented Mar 23, 2015

Hey @chkuendig, would you mind adding some tests for these new options?

@ghaiklor
Copy link
Collaborator

@chkuendig seems that tests on Travis CI is successful, but anyway, it's new features. Please, cover them with tests. Thank you.

@chkuendig
Copy link
Author

Hi @drudge, Hi @ghaiklor
i added some simple tests to cover these two options.
Regards

  • Christian

@chkuendig
Copy link
Author

anything missing before you can merge this?

@chkuendig
Copy link
Author

merged recent changes in upstream.

anything holding this back from being merged?

if (this._unpaginateFields.length > 0) {
var unpaginateFields = this._unpaginateFields;

var unpaginateField = function(pointer) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that this fn has side-effects and located in closure

@ghaiklor
Copy link
Collaborator

@drudge any suggestions?
@chkuendig if you move unpaginate to prototype and make it clean then we can merge it.

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

Successfully merging this pull request may close these issues.

3 participants