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 ws_protocol option #494

Closed
wants to merge 3 commits into from
Closed

Conversation

amrbashir
Copy link
Contributor

@amrbashir amrbashir commented Feb 15, 2023

Checklist

  • Updated CHANGELOG.md describing pertinent changes.
  • Updated README.md with pertinent info (may not always apply).
  • Updated site content with pertinent info (may not always apply).
  • Squash down commits to one or two logical commits which clearly describe the work you've done. If you don't, then Dodd will 🤓.

Sometimes it is necessary to use ws: instead of wss:, this was encountered while working on a project for tauri (on android) where using wss: wasn't possible.

Copy link
Member

@thedodd thedodd left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to open this PR! I think this is a valuable changeset. A few things which I think we should do:

  • Let's update the website to document this new configuration option.
  • We should use an enum in the Rust code to declare the two available values, either Ws or Wss.
  • If the value is left unspecified, then this should default to the current behavior where we match the site's http/https protocol.
  • In the autoreload.js, instead of checking for the interpolated {{protocol}} value of 'auto', we should just interpolate an empty string if left unspecified, and then trigger the http/https matching behavior.

Thoughts?

@amrbashir
Copy link
Contributor Author

I added all required changes except this:

Let's update the website to document this new configuration option.

The website doesn't contain all configuration options and I am not sure where to include this one, isn't it enough to include it in the example configuration file?

@github-actions
Copy link

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Oct 17, 2023
@amrbashir
Copy link
Contributor Author

Is there anything left to do before this PR can get merged?

@github-actions github-actions bot removed the Stale label Oct 18, 2023
@ctron
Copy link
Collaborator

ctron commented Oct 24, 2023

@amrbashir unfortunately I can't easily cherry-pick this into trunk-ng due to recent changes. If you want to rebase and raise a PR with trunk-ng, I could merge it.

Copy link

github-actions bot commented Dec 9, 2023

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Dec 9, 2023
@ctron
Copy link
Collaborator

ctron commented Dec 10, 2023

IIRC, this was already merged into trunk-ng.

@github-actions github-actions bot removed the Stale label Dec 11, 2023
@ctron
Copy link
Collaborator

ctron commented Dec 12, 2023

This was brought back to trunk with PR #623 and it should be part of the next release of trunk too.

@amrbashir
Copy link
Contributor Author

nice, I guess I can close this now

@amrbashir amrbashir closed this Dec 12, 2023
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.

3 participants