Token ClientOption should allow Token, TokenDetails or TokenRequest

[mattheworiordan]

Currently ClientOption#token only supports a Token or TokenDetails object. It should also support a TokenRequest given authUrl and authCallback both support all three

[mattheworiordan]

Optionally a client library should be instanceable with just a TokenDetails object, like we do for API Key string or Token string, where the language permits

[SimonWoolf]

I'm happy with the first (initing with {token: <tokenRequest>}) 👍

Not sure about the second (initing with just a tokenDetails) -- means in weakly-typed languages we'd have to guess whether an object passed to the constructor is a tokenDetails or a clientOptions. {tokenDetails: <tokenDetails>} isn't much to ask

[mattheworiordan]

Means in weakly-typed languages we'd have to guess whether an object passed to the constructor is a tokenDetails or a clientOptions.

No, you only need to check if:

• It's a string it's a token string
• If it's an object, check if it has a token attribute, then it's a TokenDetails
• Else if it's an object, it's a TokenRequest object.

That same logic exists for authUrl now.

Not sure why you'd need to determine if its a ClientOptions or not?

[SimonWoolf]

@mattheworiordan

Not sure about the second (initing with just a tokenDetails) -- means in weakly-typed languages we'd have to guess whether an object passed to the constructor is a tokenDetails or a clientOptions

That same logic exists for authUrl now. Not sure why you'd need to determine if its a ClientOptions or not?

Your're justifying your first proposal (initing with {token: <tokenRequest>}). I completely agreed with that, which is why I 👍'd it. My objection was to your second proposal, that "Optionally a client library should be instanceable with just a TokenDetails object, like we do for API Key string or Token string".

If you instance a client library with an object, currently that's assumed to be a clientOptions (in weakly-typed languages). clientOptions and tokenDetails can both have token and clientId attributes. We could guess based on the presence of expires or issued, but that's starting to get messy. (Especially if we take it it to its logical conclusion and allow initing with a bare tokenRequest object too - and if we don't, we've just introduced an inconsistency of the same kind that we've just removed with your first proposal!). {token: <tokenDetails>/<tokenRequest>} isn't too much to ask.

(I'd have no objection to the second proposal if it applies only to strongly typed languages where there's no need for guesswork)

[tcard]

ClientOption#token [...] should also support a TokenRequest given authUrl and authCallback both support all three

👍

(I'd have no objection to the second proposal if it applies only to strongly typed languages where there's no need for guesswork)

I believe that would be the first time we support different things in different kinds of languages. I don't think it's worth it to break it, it's potentially a slippery slope.

[tcard]

Additionally, we can support it everywhere but define that in JavaScript and friends the argument is considered a TokenDetails or TokenRequest only if instanceof, ClientOptions otherwise.

[mattheworiordan]

@SimonWoolf sorry, my comment Optionally a client library should be instanceable with just a TokenDetails object, like we do for API Key string or Token string, where the language permits was flippantly suggested back in Jan! I didn't even notice that and was only focussed on #75 (comment). I don't think we need to support a token object, just a token string.

Sorry for the confusion both.

[SimonWoolf]

we can support it everywhere but define that in JavaScript and friends the argument is considered a TokenDetails or TokenRequest only if instanceof

👎 TokenDetails and TokenRequest objects in the js lib are currently plain JS objects ducktyped by their properties. There's no prototype. That's probably not something we'd be looking to change at this point - for one thing that's how the js world (especially frontend devs) seems to generally expect things to work, for another that'd move 0.9 from being a technically-breaking change (that will still work for 95% of people with no changes), to being a this-will-definitely-break-your-code change.

And for what goal? The only point of being ably to do new Ably.Realtime(tokenRequest) rather than new Ably.Realtime({token: tokenRequest}) would be to save a few characters for simplicity. If we have to do new Ably.Realtime(Ably.Realtime.TokenRequest.fromJson(tokenRequest)), that's worse for simplicity than what we started with.

In any case, we can't use instanceof due to how we're using node VM contexts; see past discussion here.

[SimonWoolf]

Anyway, given Matt's clarification, this is a moot point. We all agree with the original suggestion (#75 (comment)) 🙂

[mattheworiordan]

Ok, so we are all agreed then :) @SimonWoolf fancy a spec PR?

[funkyboy]

Implemented in iOS:

• https://github.com/ably/ably-ios/blob/76056922296439e2a7fb7e90b07c2396a900354c/Spec/Auth.swift#L1039
• https://github.com/ably/ably-ios/blob/76056922296439e2a7fb7e90b07c2396a900354c/Spec/Auth.swift#L1020

Implemented in Java:

• https://github.com/ably/ably-java/blob/2af72fb88e0d5e0b47820c1c1cab6a2235f5df73/lib/src/test/java/io/ably/lib/test/rest/RestAuthTest.java#L340

Implemented in JS:

• Allow clientoptions.token to be a string, tokendetails, or tokenrequest #207

[SimonWoolf]

This was fixed by #207 and merged into the 1.1 integration branch #258