Skip to content

Support ui.run(cache_control_directives=) #4540

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

evnchn
Copy link
Collaborator

@evnchn evnchn commented Mar 28, 2025

#4532 (comment)

TL-DR:

  • Cache the static assets more aggressively, but we don't know how much is enough, and afraid overdo it.
  • Expose cache_control_directives argument in ui.run and ui.run_with, with default being long 1-year cache*
  • Better than original solution because:
    • Centralized management, as @falkoschindler pointed out
    • Ability to set other directives such as immutable (not available for Chrome), stale-while-revalidate (available for Chrome)

Concerns:

  • Is ui.run and ui.run_with the best place to add the parameter?
  • *Should we set the default to "public, max-age=3600" to match the default behaviour from before the PR (maximize compatibility), or let the better (?) 1-year-cache be the default (maximize rollout of performance increase)?

Testing:

ui.run()
{28E98406-CA70-42CE-94FA-8C894D1E6E5C}
ui.run(cache_control_directives='no-store')
{40FAC44F-07B0-44B8-910D-F0A31875726A}

@falkoschindler falkoschindler self-requested a review March 30, 2025 22:27
@falkoschindler falkoschindler added feature Type/scope: New feature or enhancement review Status: PR is open and needs review labels Mar 30, 2025
@falkoschindler falkoschindler added this to the 2.14 milestone Mar 30, 2025
Copy link
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

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

Thanks, @evnchn!
I moved the new parameter a few lines up to group it with other durations like the reconnect timeout.
Apart from that I have two open questions:

  • The docstrings say "cache control directives for static files". But methods like app.add_static_files have a separate max cache age which still defaults to an hour. Therefore I'm searching for a better description.
  • The middleware matches paths starting with "/nicegui/...". But I'm not 100% sure if this works in combination with a path prefix, like in https://github.com/zauberzeug/nicegui/tree/main/examples/nginx_subpath.

Last but not least, I think we should block this PR until PR #4539 ("Cachebusting") is merged. Changing the cache to one year without handling custom components would cause quite some trouble.

@falkoschindler falkoschindler added blocked Status: Blocked by another issue or dependency 🟡 medium Priority: Relevant, but not essential and removed review Status: PR is open and needs review labels Apr 1, 2025
@evnchn
Copy link
Collaborator Author

evnchn commented Apr 1, 2025

  1. Sorry for that oversight. Should be "cache control directives for NiceGUI internal files"
  2. According to the config, a request made to http://nginx.server/nicegui/hello would be relayed to http://localhost:8080/hello.
        location ~ ^/nicegui/(.*)$ {
            proxy_http_version 1.1;
            proxy_set_header Upgrade $http_upgrade;
            proxy_set_header Connection $connection_upgrade;
            proxy_set_header Authorization $http_authorization;
            proxy_pass_header Authorization;

            proxy_pass http://app:8080/$1?$args;
            proxy_set_header X-Forwarded-Prefix /nicegui;
            proxy_redirect http://app:8080/ /nicegui/;
  • It is mentioned that the entire app is available below "/nicegui" but the code does not need to know that., so the only way the line if request.url.path.startswith(f'/_nicegui/{__version__}/'): fails, is if it parses X-Forwarded-Prefix and mutates the path, which would be a security vulnerability (what's to stop me sending such header directly to a bare NiceGUI server, cough cough Next.js 🤡), and just doesn't make any sense.

@evnchn
Copy link
Collaborator Author

evnchn commented Apr 1, 2025

Final note: If you want to add this PR before cachebusting PR is merged, then set the default parameter of cache_control_directives to be exactly what would be served before this PR (public, max-age=3600)

But I persoanlly don't recommend rushing it. Seems a bad look if we add a parameter only that the default value changes from one version to another in a minor release.

@falkoschindler falkoschindler modified the milestones: 2.14, 2.15 Apr 7, 2025
@falkoschindler falkoschindler modified the milestones: 2.15, 2.16 Apr 16, 2025
@falkoschindler falkoschindler modified the milestones: 2.16, 2.x Apr 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Status: Blocked by another issue or dependency feature Type/scope: New feature or enhancement 🟡 medium Priority: Relevant, but not essential
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants