-
Notifications
You must be signed in to change notification settings - Fork 50
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
Feat: optional indexing and caching of site.pages
#104
Feat: optional indexing and caching of site.pages
#104
Conversation
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.
While I am not licensed to LGTM anymore 😁 , I did review this PR and it looks great.
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 good to me, thanks for the improvement!
There are a few small changes, but nothing major.
LGTM
src/template.rs
Outdated
@@ -20,6 +20,7 @@ pub struct SiteInfo { | |||
pub base_url: Option<String>, | |||
pub about: Option<String>, | |||
pub theme: Option<String>, | |||
index_site_pages: Option<Vec<String>>, |
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.
I think this should be a public field as well.
src/template.rs
Outdated
@@ -186,7 +194,12 @@ impl<'a> Renderer<'a> { | |||
// 3. ??? | |||
// 4. Leave it like it is | |||
// 5. Determine that this is out of scope | |||
pages: crate::content::all_pages(self.content_dir.clone(), self.show_unpublished)?, | |||
pages: match index_site_page { |
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.
I think an if/else
statement here might make it simpler.
src/template.rs
Outdated
@@ -171,6 +174,11 @@ impl<'a> Renderer<'a> { | |||
request_headers.insert(String::from(key.as_str()), String::from(val)); | |||
} | |||
|
|||
let index_site_page: bool = match &info.index_site_pages { |
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 could probably be a single match
statement below instead.
9f3068a
to
384e80a
Compare
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.
Noticed some spots to update to use index_site_pages
which appears to be the config key we've settled on...
Signed-off-by: Karthik Ganeshram <karthik.ganeshram@fermyon.com>
384e80a
to
4cf0eec
Compare
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.
Performance improvements look great! It looks like @radu-matei's feedback has been addressed. LGTM.
Let's remember to point this breaking change out in the forthcoming release notes....
This is a breaking change with a change required in
site.toml
to fix.Initial implementation of having the ability to index content for
site.pages
for only required templates. Implement the cache proposed in #15 forsite.pages
to improve performance.Optional indexing improves the performance of pages that do not depend on
site.toml
while caching improves the performance of any that uses it.The templates for which
site.pages
list is required can be specified insite.toml
asTODO: Docs.
Some initial performance results for tests on fermyon.com
Optional indexing
This branch
main branch
Caching
site.pages
This branch
main branch
Note: To test locally on fermyon.com add the following to
site.toml
when usingbartholomew.wasm
module from this branch to test caching with either homepage or blog page.