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 staticfiles packages with directory #1350

Merged
merged 5 commits into from Dec 7, 2021

Conversation

aminalaee
Copy link
Member

@aminalaee aminalaee commented Dec 3, 2021

Closes #1265.

Adds ability to specify directory when using the packages argument in StaticFiles.

The final API is:

routes=[
    ...
    Mount('/static', app=StaticFiles(packages=['bootstrap4', ('mypackage', 'dist')]), name="static"),
]

This will look for "statics" directory in "bootstrap4" package and "dist" directory in "mypackage".

@aminalaee aminalaee requested a review from a team December 4, 2021 11:57
@florimondmanca
Copy link
Member

florimondmanca commented Dec 5, 2021

Hi @aminalaee

As I understand it, with this PR StaticFiles(packages=["external"], directory="static") would look for static assets in /path/to/external/static, correct?

But I don't think that's what we want. Currently, the behavior is that StaticFiles(directory=..., packages=...) adds directory and one directory per package — we can see from from how directories is populated in the function at hand here.

I'm wondering if we should be considering this API instead:

StaticFiles(
    directory=...,  # An independent directory that's unrelated to `packages` (as currently)
    packages=[
        "bootstrap4",  # Default, resolves to bootstrap4/statics
        ("whatevs", "dist"),  # Explicit subdir, resolves to "whatevs/dist"
    ],
)

And then do:

for package in packages or []:
    try:
        package, statics_directory = package
    except ValueError:
        statics_directory = "statics"

    ...

@aminalaee
Copy link
Member Author

aminalaee commented Dec 5, 2021

@florimondmanca Yes you are right. I have mixed directory and packages and the API will be:

StaticFiles(
    directory="statics",
    packages=["bootstrap"],
)

And this will look for "statics" directory inside the "bootstrap" package.

There's a point we didn't take into account, we don't allow sub directories.
Maybe we should consider this too, directory is independant as you said:

StaticFiles(packages=["bootstrap.dir.statics"])

I'm happy to change this PR if we have a conclusion.

@florimondmanca
Copy link
Member

@aminalaee My API suggestion is on the same line than your second suggestion, yes — allowing to specify a per-package target sub-directory. I'd say let's do a single path item and avoid the problem of dealing with OS-dependent path representation for now, which is why I suggested a "string or 2-tuple" representation. WDYT?

@aminalaee
Copy link
Member Author

aminalaee commented Dec 6, 2021

@florimondmanca I was thinking to handle the sub-directories in an OS-independant way before another issue comes up mentioning that.

I think that makes sense for now, I'll make directory independent and go with the tuple option.

starlette/staticfiles.py Show resolved Hide resolved
starlette/staticfiles.py Show resolved Hide resolved
Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

Superb 💯

starlette/staticfiles.py Show resolved Hide resolved
@florimondmanca
Copy link
Member

I left a comment on #1265 in case anyone has practical feedback on this, before we merge and include this in the next Starlette release.

@paulmsmith
Copy link

@florimondmanca That looks like it would solve the issue myself and @bram2000 were having in #1265 👍🏻

@aminalaee aminalaee merged commit f53faba into master Dec 7, 2021
@aminalaee aminalaee deleted the fix-staticfiles-package-with-directory branch December 7, 2021 09:04
@wanghaisheng
Copy link

@florimondmanca
image

I have confused
app.mount("/static", StaticFiles(directory="./static"), name="static")

python main.py is ok, when I bundle py to a exe., I even mapping a static folder in the root folder, how to specify this directory value is the best

image

@wanghaisheng
Copy link

if. I use this. ,still not found

File "/Users/wenke/github/tiktoka-studio-uploader-genius/src/app/fastapiserver.py", line 31, in
app.mount("/static", StaticFiles(directory="static"), name="static")
File "/Users/wenke/github/tiktoka-studio-uploader-genius/.venv/lib/python3.9/site-packages/starlette/staticfiles.py", line 57, in init
raise RuntimeError(f"Directory '{directory}' does not exist")
RuntimeError: Directory 'static' does not exist

@aminalaee
Copy link
Member Author

If I understand correctly you are shipping your package as an installable, so you would need to use the StaticFiles(packages=["name_of_my_package"]) here.,

@wanghaisheng
Copy link

wanghaisheng commented Dec 14, 2023

if i understand correctly, there is 2 options for me here,
option 1 :treat static as a folder in my own package,

src/app/static

you should use this,

app.mount("/static", StaticFiles(packages=[('src.app','static')]), name="static")

the other option is in the root folder of project.
/static

app.mount("/static", StaticFiles(directory="static"), name="static")

it seems you can have directory and packages both there

app.mount("/static", StaticFiles(directory="static",packages=[('src.app','static')]), name="static")

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.

StaticFiles support for directories other than "statics"
4 participants