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

Actix web 4 (beta) support for actix-plus-static-files #1

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

finnbear
Copy link

@finnbear finnbear commented Sep 19, 2021

At some point, you should consider porting to actix-web 4 (It is in beta, so no pressure to merge this right now, but I needed to make the transition as it uses tokio >= 1).

You could model your changes on this pull request (which worked for me).

Note: I changed the mime type handling to use Mime instead of String. This helped get rid of a deprecation warning, but may be less efficient.

Thanks for the useful crate :)

Edit: I have to fix a bug preventing all but the index.html from loading. Seems like the assertion assert!(ResourceDef::prefix("/").is_match("/build/bundle.js")); failing is the cause of the problem. Maybe ResourceDef is broken. Don't merge yet.

Edit: I pushed a mitigation for the change to ResourceDef, but users who specified "/" as the path will now have to use "" or "/*"

@john01dav
Copy link
Owner

Thanks for the PR and for the interest in the library! I do have some concerns, however, that I'd like to discuss.

  • It looks like your PR just updates actix-plus-static-files, but I'd like to keep all of actix plus on the same version of actix, so ideally it'd be an update all the libraries or update none.
  • I'm not sure if I want to be gating actix plus developments behind using a beta actix version, however the pressure to upgrade due to newer tokio runtimes is large, so it might be worth it. To reiterate, I'm not saying that I won't accept an upgrade to beta, just that it's not immediately obvious to me that it's the right thing.

@finnbear finnbear changed the title Actix web 4 (beta) support. Actix web 4 (beta) support for actix-plus-static-files Sep 24, 2021
@john01dav
Copy link
Owner

Now that Actix 4 is released I'm willing to merge this once you consider it ready, and once it targets the release version on crates.io.

@finnbear
Copy link
Author

Now that Actix 4 is released

Yes, very exciting 🎉

Thank you for your interest in merging this PR. I've just updated it to actix-web stable 4.0. Please note: Since your first comment, this PR also adds a feature to pre-compress static files (except compressed image formats and files under 1400 bytes) using gzip. So this PR is not as simple as actix-web 3.0 -> 4.0.

@john01dav

@john01dav
Copy link
Owner

john01dav commented Feb 28, 2022

Thanks for the PR (the compression sounds like a great addition), however I cloned this repository to do a quick sanity check that everything works, and there seem to be some bugs, although perhaps I am missing something. Either way, I need to discuss this with you prior to merging. Firstly, I needed to add the default features of actix-web to dev dependencies in order for the basic_use example to compile at all (actix_web::main is behind a default feature), and, secondly, it looks like the nested-file-test/alert.js file is not being served properly, as it gets a 404. I really should make these proper automated tests at some point, but there does nonethless seem to be a bug here.

@finnbear
Copy link
Author

Thanks for the PR (the compression sounds like a great addition), however I cloned this repository to do a quick sanity check that everything works, and there seem to be some bugs, although perhaps I am missing something. Either way, I need to discuss this with you prior to merging. Firstly, I needed to add the default features of actix-web to dev dependencies in order for the basic_use example to compile at all (actix_web::main is behind a default feature), and, secondly, it looks like the nested-file-test/alert.js file is not being served properly, as it gets a 404. I really should make these proper automated tests at some point, but there does nonethless seem to be a bug here.

I didn't try any of your tests (I just made sure my app worked) 😂

I suspect it may have something to do with the upstream change to how paths are matched.

I'll look into this in the next few days!

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.

2 participants