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

JSON support for Collection API #167

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

Conversation

y-ich
Copy link

@y-ich y-ich commented Jun 8, 2016

Hi.

Recently, I tried to use Twitter's Collections API and found that it needed Content-Type of application/json for collections/entries/curate.
(See https://dev.twitter.com/rest/reference/post/collections/entries/curate.)

Extending your library to support application/json was quite easy and this is a patch.

Usage:

var Twitter = require('twitter');

var client = new Twitter({
    consumer_key: '',
    consumer_secret: '',
    access_token_key: '',
    access_token_secret: '',
    request_options: { json: true }
});

var params = {id: 'XXX', changes: [{ op: 'add', tweet_id: 'YYY' }]};
client.post('collections/entries/curate', params, function(error, data, response){
    if (!error) {
        console.log(data);
    }
});

One problem is that the API is not smart because you need to specify json type when creating a Twitter instance, so the instance can be used just for application/json type request.

I am not quite sure which is better, adding json option when instantiating and checking it in __request method, or just checking if endpoint is collections/entries/curate in __request method.

@Objelisks
Copy link

Hi, if someone updates the docs to reflect this, the change is also useful for supporting https://dev.twitter.com/rest/reference/post/media/upload

@svangordon
Copy link

I was wondering if you could clarify what you mean by

the instance can be used just for application/json type request.

I'm not super familiar with the Twitter API. Does this mean that a client instantiated with the request_options: { json: true } will only work on certain endpoints?

@desmondmorris
Copy link
Owner

@y-ich - as @svangordon as points out, by making this a instance level configuration, you would need to create a different instances for the different types of requests. Also, the request_options object is passed along to the request module, so placing a non request specific config in there, may cause side effects.

@y-ich
Copy link
Author

y-ich commented Dec 12, 2016

@svangordon san, @desmondmorris san, I am so sorry to miss your comments for a long time.

Twitter's Collections API needs an array to specify some properties, so usual query form is not enough, that supports only string-keys and string-values pairs.

And I have no idea whether normal post request sends JSON as JSON or as query parameter string.
So when you specify request_options: { json: true } on my pull request, I am not sure that it may work for normal Twitter APIs. So you may need two instances, one is for normal Twitter APIs and the other is for Twitter Collection API.

Passing request_options: { json: true } as an argument for post method as another implementation will enable one instance to work for both APIs and will eliminate the internal configuration.

@desmondmorris
Copy link
Owner

@y-ich we should find a way to avoid forcing multiple instances. We also should not hijack the request options object as it is passed directly to the request npm module. Basically, we should not break experiences for most cases to accommodate a few.

Perhaps, we should only change the request to deliver JSON on this particular endpoint? It is not desirable, but the cost at at this point in time is less than the alternative.

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.

None yet

4 participants