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

Make file serving helpers public #924

Closed
wants to merge 6 commits into from

Conversation

arctic-hen7
Copy link

This makes some of the contents of fs.rs public so that users can serve static files at paths based on the request without having to implement file streaming manually.

Fixes #171.

@arctic-hen7
Copy link
Author

@seanmonstar I'm slightly unsure about how you want some of these internals documented. I've mentioned you in the code for Conditionals, the fields of which I haven't documented yet.

@arctic-hen7 arctic-hen7 marked this pull request as ready for review December 11, 2021 23:58
arctic-hen7 added a commit to framesurge/perseus that referenced this pull request Dec 12, 2021
None of this has been tested yet, so there will likely be bugs.
We now depend on my fork of Warp until [this](seanmonstar/warp#924) is merged.
arctic-hen7 added a commit to framesurge/perseus that referenced this pull request Dec 12, 2021
* refactor: moved some functions for integrations into the core package

This will make developing more integrations in future much simpler.
This also involved standardizing the `Options` taken by all integrations

BREAKING CHANGE: `Options` renamed to `ServerOptions` for all integrations

* feat: made templates and error pages thread-safe

This involved adding an atomic types system.
Also added basics for a Warp integration (which needs this thread-safety).

* feat: made more things thread-safe and made warp integration nearly work

The problem is `Rc<Translator>`s, so some refactoring needs to be done.

* feat: added nearly all handlers to warp integration

BREAKING_CHANGE: `ServerOptions` now only accepts one static content directory

* fix: made `DummyTranslator` `Clone`able

* feat: added support for static aliases in the warp integration

None of this has been tested yet, so there will likely be bugs.
We now depend on my fork of Warp until [this](seanmonstar/warp#924) is merged.

* fix: pinned `clap` version

Fixes #85.

* feat: finalized warp integration

It's also now the default in the CLI.

* docs: updated docs to reflect warp integration

* docs: merged `next` and `0.3.x` docs

* chore: extended version replacement script

Now covers the `Cargo.toml` files in `examples/basic/.perseus`.
@arctic-hen7
Copy link
Author

@seanmonstar is there anything blocking this in particular?

@arctic-hen7
Copy link
Author

Okay, not sure if anyone is going to review this but for anyone with the same problems as in #171 I published a temporary fix crate (I needed this immediately) as warp-fix-171 on crates.io. Hope this helps someone other than me!

Obviously, if this gets merged I'll deprecate that crate.

src/filters/fs.rs Outdated Show resolved Hide resolved
src/filters/fs.rs Outdated Show resolved Hide resolved
src/filters/fs.rs Outdated Show resolved Hide resolved
@tobz
Copy link

tobz commented Feb 3, 2022

Yep, the comments look a lot better now and will be very useful to developers. 👍🏻

@arctic-hen7
Copy link
Author

@seanmonstar are there any plans to merge this? I'm very happy to update this to be in sync with master if so.

@quinn
Copy link

quinn commented Nov 22, 2022

@seanmonstar would be great to get this merged! blocking something i'm working on 🙏

@arctic-hen7
Copy link
Author

@quinn if it helps there's my public warp-fix-171 crate, which has this fix. Unless you need the latest features from Warp itself, which that doesn't have.

@arctic-hen7
Copy link
Author

@seanmonstar I notice the repo has had some activity lately, any chance of this being merged? I've updated everything to be in sync with the master branch.

(For others: until this is merged, I've released another version of the warp-fix-171 patch crate.)

Copy link
Owner

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

I think adding a filter to get Conditionals seems like a good idea! I'd suggest a little different though: Add a method to Conditionals, that is fn reply<P: AsRef<Path>>(self, path) -> impl Reply.

#[derive(Debug, Default)]
pub struct Conditionals {
/// Only respond with the file if it has been modified since this date. [See MDN](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/If-Modified-Since).
pub if_modified_since: Option<IfModifiedSince>,
Copy link
Owner

Choose a reason for hiding this comment

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

I would keep the fields private. That way we can add more, or change them. We also haven't exposed the headers crate publicly.

#[derive(Clone, Debug)]
struct ArcPath(Arc<PathBuf>);
pub struct ArcPath(pub Arc<PathBuf>);
Copy link
Owner

Choose a reason for hiding this comment

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

I would keep this private.

///
/// Usually, you'll want to use this with `warp::fs::file`, but if the file's path is based on something extracted with a filter,
/// you'll need to use this manually.
pub fn file_reply(
Copy link
Owner

Choose a reason for hiding this comment

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

Also kept private.

@arctic-hen7
Copy link
Author

Yeah you're right that is much cleaner, I'll refactor this PR as soon as I can.

(Terribly sorry for the late reply by the way, I've been really busy the last month!)

@arctic-hen7
Copy link
Author

Closing because #1049 supersedes this (according to its description).

@arctic-hen7 arctic-hen7 closed this Sep 8, 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.

Add reply::file(path) helper
5 participants