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

Added graph-video option #185

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

daniellobrunello
Copy link

Added the graphVideo option for uploading video material to the graph-video.facebook.com endpoint.
Example:

FB.options({videoGraph: true});
FB.api(123456789 + '/videos', 'post', {
    title: 'Test Video',
    published: true,
    access_token: 'ABCDEFGHIJKLMOP12345!?',
    file_url: 'https://vids.test.com/test.mp4'
});

@dantman
Copy link

dantman commented Aug 14, 2020

Not sure I like the videoGraph option. This seems like something that would be better as part of the path arg to the api function. So you don't need two entirely separate SDK instances to make video and non-video API requests.

@daniellobrunello
Copy link
Author

You don't need two separate instances, you just have to switch the option before making the API request. Its not the most elegant way, I have to admit.
So you would prefer a check for a complete TLD for the path arg?

@dantman
Copy link

dantman commented Aug 21, 2020

You don't need two separate instances, you just have to switch the option before making the API request. Its not the most elegant way, I have to admit.

API requests are async. So that is just begging for race condition based bugs.

So you would prefer a check for a complete TLD for the path arg?

Yeah, I'd prefer some way of sending a request to the video api using the first arg to the method. Maybe a video: prefix, an object arg like { video: true, path: ... } or { api: 'video', path: ... }. Or perhaps we should go with FB.videoApi. A video option might be acceptable if it's a hidden prop where you essentially do FB.video.api(...) (FB.video would internally extend the instance and set the api to 'video').

Thinking about it FB.video.api sounds like it might be the nicest API.

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.

2 participants