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

feat: Add PlaybackGrant. #700

Merged
merged 1 commit into from
Oct 14, 2021

Conversation

sarahcstringer
Copy link
Contributor

Fixes

Adds PlaybackGrants.

Checklist

  • I acknowledge that all my contributions will be made under the project's license
  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the Contribution Guidelines and my PR follows them
  • I have titled the PR appropriately
  • I have updated my branch with the main branch
  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary documentation about the functionality in the appropriate .md file
  • I have added inline documentation to the code I modified

If you have questions, please file a support ticket, or create a GitHub Issue in this repository.

Copy link
Contributor

@stern-shawn stern-shawn left a comment

Choose a reason for hiding this comment

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

🚢 🇮🇹

@@ -177,6 +177,22 @@ declare namespace AccessToken {
endpointId?: string;
}

export interface PlaybackGrantOptions {
grant?: object;
Copy link
Contributor

@stern-shawn stern-shawn Oct 14, 2021

Choose a reason for hiding this comment

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

Based on this sample from the test, I think we should look into typing grant more specifically since object is a very loose definition.

var playbackGrant = {
  requestCredentials: null,
  playbackUrl: 'https://000.us-east-1.playback.live-video.net/api/video/v1/us-east-000.channel.000?token=xxxxx',
  playerStreamerSid: 'VJXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX',
};

var grant = new twilio.jwt.AccessToken.PlaybackGrant();
grant.grant = playbackGrant;

Maybe a new interface to DRY things up?:

export interface PlaybackGrantResponse {
  requestCredentials: string | null;
  playbackUrl: string;
  playerStreamerSid: string;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

+1. Also need to do the same in other languages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks so much for the suggestion -- we chatted and are planning to do this step in a separate PR, to speed up getting these into the helper libraries. But definitely going to make a new PR to clean this up.

Choose a reason for hiding this comment

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

The playback grant is supposed to be an opaque object returned by a Live API, none of these fields have to be provided by the developer. While adding typing seems like a good idea in principle, this is going to break if/when the Twilio Live team decide to make changes to the format of the grant, like maybe add a new field. I'm not really sure what the best option is, just wanted to point out that there are downsides to adding typing about private attributes of the grant.

@shwetha-manvinkurke shwetha-manvinkurke changed the title [feat] Add PlaybackGrant. feat: Add PlaybackGrant. Oct 14, 2021
@sarahcstringer
Copy link
Contributor Author

I was able to verify here that I could add a PlaybackGrant to a token and connect with it:

const AccessToken = require('twilio').jwt.AccessToken;
const PlaybackGrant = AccessToken.PlaybackGrant;

const playbackGrant = new PlaybackGrant({
  grant: {'requestCredentials': null,
 'playbackUrl': 'https:/...',
 'playerStreamerSid': 'VJXXX'}
});

const token = new AccessToken(
  twilioAccountSid,
  twilioApiKey,
  twilioApiSecret
);
token.addGrant(playbackGrant);

// Serialize the token to a JWT string
console.log(token.toJwt());

@shwetha-manvinkurke shwetha-manvinkurke merged commit 9d256dd into twilio:main Oct 14, 2021
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.

4 participants