Support wss protocol for change streamer#4555
Conversation
|
@tjenkinson is attempting to deploy a commit to the Rocicorp Team on Vercel. A member of the Team first needs to authorize it. |
tjenkinson
changed the title
Adds a `changeStreamer.protocol` (`ZERO_CHANGE_STREAMER_PROTOCOL`) option, which can be set to `https` Before rocicorp#4335 we were able to use a https url.
tjenkinson
force-pushed
the
change-streamer-protocol
branch
from
eb1d154 to
d905415
Compare
tjenkinson
changed the title
There was a problem hiding this comment.
Recommend ignoring whitespace. Wrapped all the existing tests in another describe so can have one for http and the other for https
| throw new Error(`no change-streamer is running`); | ||
| } | ||
| const uri = new URL(path, `http://${address}/`); | ||
| const uri = new URL( |
There was a problem hiding this comment.
not tested currently. Not sure on the best approach given the current tests rely running a local server. Maybe need to start a https server too and disable certificate validation?
| const uri = new URL(path, `http://${address}/`); | ||
| const uri = new URL( | ||
| path, | ||
| address.startsWith('http://') || address.startsWith('https://') |
There was a problem hiding this comment.
Maybe: address.includes("://") so that it takes any scheme/protocol into account? Not that we expect people to use it, but ws:// and wss:// would also be valid.
There was a problem hiding this comment.
Hey @darkgnotic !
Sure can update tomorrow. Should I allow the others in the config too then?
There was a problem hiding this comment.
Actually is http/https even valid for a websocket? Maybe it should be ws/wss?
There was a problem hiding this comment.
Looks like really it should be ws/wss but TIL support for http/https was added to support relative urls whatwg/websockets#45 websockets/ws#2162
I also had /^[a-zA-Z0-9+\-.]+:/.test(address) ? `${address}/` : `ws://${address}/`, locally to accept any protocol here, but if the config only accepts ws/wss maybe it's better to be stricter?
| await db`UPDATE ${this.#cdc('replicationState')} SET ${db({owner, ownerAddress})}`; | ||
| this.#lc.info?.(`assumed ownership at ${ownerAddress}`); | ||
| const ownerProtocol = this.#discoveryProtocol; | ||
| // we omit `http://` so that old view syncer versions that are not expecting the protocol continue to not get it |
There was a problem hiding this comment.
Well thought out. Thank you!
tjenkinson
force-pushed
the
change-streamer-protocol
branch
from
7554d37 to
a7f094e
Compare
tjenkinson
changed the title
tjenkinson
force-pushed
the
change-streamer-protocol
branch
from
a7f094e to
e34bd8e
Compare
darkgnotic
left a comment
darkgnotic
left a comment
There was a problem hiding this comment.
Thank you @tjenkinson !
| const uri = new URL(path, `http://${address}/`); | ||
| const uri = new URL( | ||
| path, | ||
| address.startsWith('ws://') || address.startsWith('wss://') |
There was a problem hiding this comment.
Limiting the protocol value to ws and wss in the option definition seems fine, but I would prefer leaving this part to be a scheme agnostic check of includes('://').
Juuust in case something out there depends on the scheme being http. Hopefully nothing does. 🤞
There was a problem hiding this comment.
Done! Although I'm not sure how something else could get into the db?
There was a problem hiding this comment.
The idea is that it would be easier to add more options if necessary (fewer places in the code to modify).
Thank you! I'll merge this!
2 participants
Adds a
changeStreamer.protocol(ZERO_CHANGE_STREAMER_PROTOCOL) option, which can be set towssBefore #4335 we were able to use a https url.
With this change if https is used, the
ownerAddressvalue in the db will have thewss://suffix. The view syncer will use the protocol in the url if there is one, otherwise default tows://I decided to still not write
ws://into the URL in the db to not break existing deployments, as if a new version of the replication manager came up first, an existing view syncer could crash.Deployed to our staging env and seems to work