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

Generate sitemap for dynamic deploy url #4923

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bhatia2akshit
Copy link

@bhatia2akshit bhatia2akshit commented Mar 7, 2025

All Submissions:

  • Have you followed the guidelines stated in CONTRIBUTING.md file?
  • Have you checked to ensure there aren't any other open Pull Requests for the desired changed?

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

New Feature Submission:

  • Does your submission pass the tests?
  • Have you linted your code locally prior to submission?

Changes To Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

After these steps, you're ready to open a pull request.

a. Give a descriptive title to your PR.

b. Describe your changes.

c. Put `closes #XXXX` in your comment to auto-close the issue that your PR fixes (if such).

Copy link
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

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

This is pretty cool, a couple of suggestions,

  • the implementation should be outside of app.py and imported from there
  • consider generating the sitemap.xml during compile time and place the resulting file in the frontend assets to avoid having to generate xml for each request, when the content is unlikely to have changed.

@benedikt-bartscher
Copy link
Contributor

  • consider generating the sitemap.xml during compile time and place the resulting file in the frontend assets to avoid having to generate xml for each request, when the content is unlikely to have changed.

There is a plan to introduce dynamic sitemaps, f.e. for a blog. This would require to implement 2 different approaches, one for static sitemap and one for dynamic sitemap. We could f.e. try to auto-detect if dynamic features are used or add a config option for that.

@bhatia2akshit
Copy link
Author

bhatia2akshit commented Mar 8, 2025

@masenf running it before compile time would mean that backend must replace deploy url if its different from the ones in sitemap. I would think the better approach would be to run the sitemap generation when the app is deployed for the first time on the server. This would allow the same image to be deployed in different servers with unique sitemaps.

@bhatia2akshit
Copy link
Author

bhatia2akshit commented Mar 11, 2025

@masenf I have made changes as per your suggestions

  1. shifted sitemap generating methods into a separate file.
  2. added sitemap generation when the app is deployed on the server based on the deploy url. When a user request sitemaps, it checks if the sitemap exist in the static folder under .web directory. If not, new sitemaps are generated otherwise existing is served.
  3. add_page accept sitemap's priority (default is 10.0, signalling user didnt provide it and hence automatic needed) and changefreq (default is 'weekly')

Copy link

codspeed-hq bot commented Mar 11, 2025

CodSpeed Performance Report

Merging #4923 will not alter performance

Comparing bhatia2akshit:generate-sitemap-new (33610bd) with main (346bce0)

Summary

✅ 12 untouched benchmarks

Copy link
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

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

this looks good, but when i reflex export, it looks like .web/_static directory is deleted. it's probably better to write into .web/public.

if possible, this should remove the next-sitemap dependency as we're not using it anymore.

run pre-commit and address unit test failures

@bhatia2akshit
Copy link
Author

hould remove the next-sitemap dependency as we're not using it anymore.

When you run reflex export, the _static folder is not created. The reason being i commented the next-sitemap generation. The deployment will create the _static folder.

@bhatia2akshit
Copy link
Author

@masenf I have fixed the errors. Do you agree to my reasoning related to not needed to write to PUBLIC folder? Like, when we build our web app, the static folder is not created. when we start the app, sitemaps are generated and written in the static folder.

Copy link
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

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

This doesn't seem to work:

reflex run

http://localhost:8000/sitemap.xml -- no pages are listed


The following commands simulate a common production-like workflow and it also doesn't work with the sitemap logic as written:

reflex init
reflex export --frontend-only --no-zip
mv .web/_static /tmp/srv && (cd /tmp/srv && python3.12 -m http.server 3000) &
reflex run --backend-only
  1. http://localhost:3000/sitemap.xml is a 404
  2. http://localhost:8000/sitemap.xml does not list any pages

In a backend-only deployment, the page routes are not compiled, so app._pages is empty. That's why it doesn't really make sense to have get_pages. If you want to get all of the routes that would be compiled, you need to use app._unevaluated_pages.

I also still think it makes sense to export a static sitemap.xml with the frontend, so that it can be served even when the backend is down. If you also need to dynamically regenerate the sitemap.xml when the backend is running, i suppose that's okay too, but i still don't see the advantage of that personally.

@bhatia2akshit
Copy link
Author

@masenf Yeahh I see the mistake in the approach. I will change the approach then.

@bhatia2akshit
Copy link
Author

@masenf I have made the required changes, and tested it with reflex run. It works now.

@masenf masenf mentioned this pull request Mar 20, 2025
@bhatia2akshit
Copy link
Author

@masenf @benedikt-bartscher, Does it fulfil all the requirements? or is there something else that needs to be added?

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.

3 participants