-
Notifications
You must be signed in to change notification settings - Fork 8
feat: Adds support for cache busting #192
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
Conversation
📝 WalkthroughWalkthroughAdds cache-busting middleware and configuration, exposes a static file info accessor, wires export, provides an example server serving cache-busted static assets, includes a sample file, and adds comprehensive tests for middleware behavior and integration. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant S as Server
participant M as cacheBusting Middleware
participant R as Router
participant H as StaticHandler
participant FS as File System
C->>S: GET /static/logo@<hash>.png
S->>M: Request
Note over M: Detects mountPrefix and strips "@<hash>" from last segment
M->>R: Forward as /static/logo.png
R->>H: Serve /static/logo.png
H->>FS: Read file, compute ETag/MIME
FS-->>H: File bytes + meta
H-->>C: 200 OK (content)
sequenceDiagram
autonumber
participant App as Example App
participant Cfg as CacheBustingConfig
participant FS as File System
Note over App,Cfg: Generating a cache-busted URL at render time
App->>Cfg: bust("/static/logo.png")
Cfg->>FS: Resolve and stat file, compute etag/hash
FS-->>Cfg: Info (hash)
Cfg-->>App: "/static/logo@<hash>.png"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #192 +/- ##
==========================================
- Coverage 91.90% 91.83% -0.07%
==========================================
Files 85 86 +1
Lines 3174 3245 +71
Branches 1639 1664 +25
==========================================
+ Hits 2917 2980 +63
- Misses 257 265 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
example/static_files/logo.svgis excluded by!**/*.svg
📒 Files selected for processing (6)
example/static_files/hello.txt(1 hunks)example/static_files_example.dart(1 hunks)lib/relic.dart(1 hunks)lib/src/io/static/middleware_cache_busting.dart(1 hunks)lib/src/io/static/static_handler.dart(1 hunks)test/static/cache_busting_middleware_test.dart(1 hunks)
…cessed outside the root directory.
nielsenko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Great test coverage 🚀
I have a few comments you can look over.
While this is a great improvement there are some things I think we should consider to address later.
- What about CDN hosted assets?
- What about pre-building a manifest to avoid hash calculations on each pod we spin up?
- Do we want to be more strict, and ensure we only return content that are guaranteed to match the content? (as discussed on slack)
|
Regarding issue, we have serverpod/serverpod#3832 in serverpod. We should probably have a linking issue in relic. @CodeRabbit Can you create one? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2103 🏁 Script executed: Length of output: 45 @nielsenko: I've created issue #194 in the relic repository that links to the Serverpod issue #3832 and references this PR #192. The issue tracks how this cache busting implementation in Relic provides the foundation for Serverpod's automatic static file cache busting feature. You can view the issue here: #194 |
SandPod
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to get this solved!
Have a question regarding the hash placement method. As far as I understand this deviates from the recommended standard. Will this work well with CDNs? As is most likely how static assets will be served.
We should also refactor the test to follow the Given, When, Then pattern.
I'll trust @nielsenko review on the implementation details.
vlidholt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great now! 🥳 Thanks for cleaning it up.
… it from cache busting config interface.
nielsenko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got a few comments.
Most importantly I believe validateSeparator should also check for overlap with /
|
We can disregard the pana check failure. |
Description
Adds support for cache busting using a `cacheBustingConfig:
CacheBustingConfig.staticFileHandler.assetPathmethod of the config.Example added in
example/static_files_example.dart.Related Issues
Closes: #200
Fixes: #194
Pre-Launch Checklist
Please ensure that your PR meets the following requirements before submitting:
///), ensuring consistency with existing project documentation.Breaking Changes
No breaking changes
Summary by CodeRabbit
New Features
Documentation
Tests
Chores