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

Add config option to specify additional headers for serve #322

Closed
wants to merge 1 commit into from

Conversation

oberien
Copy link
Contributor

@oberien oberien commented Feb 17, 2022

I'm using webworkers (based on #285) and want to use SharedArrayBuffer. That requires setting the headers Cross-Origin-Opener-Policy: same-origin and Cross-Origin-Embedder-Policy: require-corp. To allow this configuration during development with trunk serve, I added a config option which allows specifying any number of header-value pairs, which are included in responses.

Fix #414

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 🤓.

@oberien oberien force-pushed the headers branch 2 times, most recently from 138d7cb to 0e98114 Compare February 22, 2022 08:10
@iTitus
Copy link

iTitus commented Jun 20, 2022

Is there any update on this?
I'd like to add CORS headers in my local dev env.

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.

One nit (which CI should have caught, we'll update later). Will be g2g after that.

src/serve.rs Outdated
@@ -19,6 +19,8 @@ use crate::common::SERVER;
use crate::config::RtcServe;
use crate::proxy::{ProxyHandlerHttp, ProxyHandlerWebSocket};
use crate::watch::WatchSystem;
use axum::http::header::HeaderName;
use std::collections::HashMap;
Copy link
Member

Choose a reason for hiding this comment

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

Let's move these imports up to where their other related imports take place. axum::http:: is already being imported, so let's group those. Then for std::collections::HashMap, let's move that up to line 1 (alphabetical order).

@oberien
Copy link
Contributor Author

oberien commented Jul 3, 2022

Thanks for the review. Unfortunately the changes have become pretty stale. I rebased and needed to completely change up the implementation. It works with the new ServeDir instead of the custom endpoint implementation now.

This time I ran rustfmt, so the imports should be in the correct order :)

@oberien oberien requested a review from thedodd July 3, 2022 15:45
@vitvakatu
Copy link

Hi, @thedodd. Is anything blocking this PR? I also faced #414 in my project. I overlooked this PR and even prepared my own. (available here, the approach is slightly different, I also updated docs and added an example)

I'd love to see this merged and released. It's quite a simple feature that helps tremendously.

@SeanOMik
Copy link

I also would love to see this merged. It looks like GitHub is saying that this branch is out-of-date with the base branch. I'll be willing to try to rebase it to the master and make other changes if needed.

@oberien
Copy link
Contributor Author

oberien commented Nov 12, 2022

Rebased to the latest master. Is there anything else I can do to help move this PR along? (The 2 clippy lints don't affect code I changed.)

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.

Very last tiny item! I'll push the merge button on this once it is done.

Thanks again for all of the great work on this.

CHANGELOG.md Outdated
@@ -40,6 +40,7 @@ Subheadings to categorize changes are `added, changed, deprecated, removed, fixe
- It is now possible to disable the hashes in output file names with the new `--filehash` flag (for example `cargo build --filehash false`). Alternatively the `build.filehash` setting in `Trunk.toml` or the env var `CARGO_BUILD_FILEHASH` can be used.
- Flags for enabling reference types & weak references in `wasm-bindgen`.
- Added the `data-typescript` attribute to Rust assets. When present, `wasm-bindgen` will emit TS files for the WASM module.
- Added `headers` config option for `trunk serve`.
Copy link
Member

Choose a reason for hiding this comment

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

We should add this to the unreleased section of the changelog. Once we are ready to cut a release, it will get moved down into a specific release version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch that the automerge resulted in this artifact. Fixed.

@chaosprint
Copy link

Thanks for this PR. Look forward to its progress.

@Cornul11
Copy link

Cornul11 commented Mar 15, 2023

Are there any updates on the possibility of merging this to the upstream?

@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 20, 2023
@ctron
Copy link
Collaborator

ctron commented Oct 20, 2023

I merged this into trunk-ng, if it looks good for the next few days, it should be released as 0.17.11.

It would be great if any of you could test this feature specifically. You need to install trunk-ng from git for doing so: cargo install --git https://github.com/ctron/trunk --branch trunk-ng

@github-actions github-actions bot removed the Stale label Oct 21, 2023
@oberien
Copy link
Contributor Author

oberien commented Oct 21, 2023

I tested trunk-ng and it seems to work just fine. I can't tell a difference in behaviour between my branch and trunk-ng.

@ctron
Copy link
Collaborator

ctron commented Oct 24, 2023

This is now released with trunk-ng 0.17.11

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 Dec 10, 2023
@oberien
Copy link
Contributor Author

oberien commented Dec 11, 2023

This PR is not stale. The feature is used by several people and has even been integrated into trunk-ng.

@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.

@ctron
Copy link
Collaborator

ctron commented Dec 13, 2023

This was released with trunk 0.18.0.

@ctron ctron closed this Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting Headers for the Trunk dev server.
8 participants