-
Notifications
You must be signed in to change notification settings - Fork 1
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
clarify that the url is a web socket #610
clarify that the url is a web socket #610
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good a few minor comments.
SIWF_NODE_RPC_URL: undefined, | ||
SIWF_URL: undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: We already have SIWF_URL
. Is this redundant or they are used differently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to also change SIWF_URL to be SIWF_UI_URL? @mattheworris @aramikm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do like SIWF_UI_URL
but I'm fine with leaving SIWF_URL
too, as you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Paired with @claireolmstead on some changes
- Read through finalized changes
- Non-blocking comments
🚢 it!
82c66bc
to
8b16c31
Compare
Sorry; just getting around to looking at this--last 2 weeks with hurricane & off-site have me in catch-up mode.
We should either disallow http protocol in the config, or eliminate specifying WebSocket/WS in the docs & env name. I'd be okay with disallowing http, since it's less efficient (needs to establish a connection with each request); however, we might want to consider that some client might have network constraints in their environment that preclude using WebSocket. |
Problem
The names of the env vales are confusing.
Link to GitHub Issue(s): #567
Solution
Clarify that one is a websocket.
Steps to Verify: