fix(multi-video-player): use more specific path split in fromPullUrl#1604
Conversation
Let maintainers know that an action is required on their side
|
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
question: Shouldn’t the base URL be everything that is before /jet? E.g.: gatewayUrl = https://gw1.somedomain.ninja:7000/
89f0e21 to
5c82063
Compare
There was a problem hiding this comment.
Upon re-reviewing this, I’m going to push back on changing the public API here.
The current shape of the API is deliberate: GatewayRecordingApi takes a Gateway base URL and knows internally that the relevant endpoints live under /jet/jrec, /jet/sessions, etc. In other words, callers give it "the thing closest to the real base" (e.g. https://gw1.somewhere.ninja/), and the class owns the knowledge of /jet/… paths.
Switching the constructor to take a jetApiUrl (e.g. https://gw1.somewhere.ninja/jet/) moves those details into the caller. That’s an abstraction leak.
On the /jet/jrec "duplication" point: I don’t really see a concrete problem there. Yes, we repeat /jet/jrec in several methods, but in this case I’d rather be explicit than DRY:
- Each method clearly shows the full path it is calling.
- There’s no confusion about which endpoints live where.
- The gain from refactoring this into a shorter base (
jetApiUrl) is small, while the cost in terms of API semantics and abstraction is somewhat high (especially, both the old and new APIs still take a string, but the value must be carefully updated; this is a silent breaking change).
So I don’t think changing the public API just to avoid repeating /jet or /jet/jrec is worth it.
That said, it seems to me that fromPullUrl on master is buggy: fromPullUrl should derive the Gateway base URL (everything before /jet), and currently it seems to split at /pull instead of /jet. But maybe I’m misunderstanding the split method of JavaScript. Can you confirm that?
If you’re OK with that direction, I would be happy to review a follow-up that only adjusts fromPullUrl and leaves the constructor signature and responsibilities as they are.
5c82063 to
4c19155
Compare
|
Thanks for the feedback Benoit! |
…uction Refactor GatewayRecordingApi to use the jet api base (/jet) instead of the jrec endpoint (/jet/jrec) as the base URL. fix(multi-video-player): remove duplicate path segments in URL construction
4c19155 to
d472540
Compare
…uction Refactor GatewayRecordingApi to use the jet api base (/jet) instead of the jrec endpoint (/jet/jrec) as the base URL.
…duplication' into fix/multi-video-player-url-path-duplication
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
LGTM! Thank you!
|
Please, update the PR title / body to reflect the changes, and feel free to merge yourself 👍 |
Change fromPullUrl to split on '/jet/jrec/pull/' instead of '/pull/' to properly support reverse proxy deployments with path-based routing