FeedHive / twitter-api-client

A user-friendly Node.js / JavaScript client library for interacting with the Twitter API.
MIT License
948 stars 84 forks source link

add retweeted_status to tweet-template #84

Closed goleary closed 3 years ago

goleary commented 3 years ago

What does this pull request introduce?

Accounts for home_timeline statuses that contain the retweeted_status prop.

closes #83

How did you verify that your changes work as expected? Please describe

I used the newly generated types in my project and they work as expected.

Example

const statuses = await client.tweets.statusesHomeTimeline({
  count: 200,
});
for (const status of stautes) {
  console.log(status.retweeted_status
}

Version
1.3.9

NOTE:

@SimonHoiberg There is an issue that needs to be resolved before this can be merged. The retweeted_status field is optional, but it's not clear to me how to mark a property as optional given that the type is generated from a single JSON template. Any suggestions?

PR Checklist Please verify that you:

SimonHoiberg commented 3 years ago

Awesome, man! Thanks a lot for opening a PR :partying_face:

Please, before we review, fill out the template description above.

In particular, please verify that you have:

Then we'll get right to reviewing it 😁

goleary commented 3 years ago

Forsure, thanks for the great package!

I updated the comment above to fill out the template (sorry for the originally incomplete PR, I had to run & should've just waited to submit it).

Included above, you'll find this note:

There is an issue that needs to be resolved before this can be merged. The retweeted_status field is optional, but it's not clear to me how to mark a property as optional given that the type is generated from a single JSON template. Any suggestions?

SimonHoiberg commented 3 years ago

Awesome, thanks a lot!

There is an issue that needs to be resolved before this can be merged. The retweeted_status field is optional, but it's not clear to me how to mark a property as optional given that the type is generated from a single JSON template. Any suggestions?

Yeah, we can't do this in our current setup. We're using a library to infer types based on JSON, and I don't think it has that capability.

But we actually have this issue in a lot of places. The types are generated merely from the example outputs that are given on Twitter API's documentation page. But in general, they are incomplete, so types are never going to be 100% accurate anyway.

I think this is fine to merge 🙌

goleary commented 3 years ago

Yeah, we can't do this in our current setup. We're using a library to infer types based on JSON, and I don't think it has that capability.

Gotcha. I'm certainly not proposing refactoring this library, but for future reference: quicktype has support for providing multiple JSON samples (i.e. with and without a property) that results in optional properties in the generated types.

Having an optional field presented as required makes me a little nervous, but if it's happening elsewhere I suppose not a big deal.

I'll make sure to be sure to check for presence of this field before attempting to use it.

Thanks for your responsiveness, looking forward to the next version of the library!

goleary commented 3 years ago

After digging into json-to-ts it looks like there is support for optional properties using a curious syntax (--? added to the end of the property). You can see it here.

I tested it with retweeted_status in tweet-template.json & it worked as expected, producing this result:

export default interface StatusesHomeTimeline {
  ...
  retweeted_status?: Retweetedstatus;
}

I can't say I love this method as it requires modifying the raw JSON being returned from Twitter & is more error prone, but it seems better than nothing.

I'll submit a PR on the development branch and you can let me know whether or not you agree.