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

Consider excluding Search component from Angular SSR (Server Side Rendering) #3231

Open
tdonohue opened this issue Aug 1, 2024 · 3 comments
Assignees
Labels
affects: 7.x Issue impacts 7.x releases affects: 8.x Issue impacts 8.x releases bug component: Discovery related to discovery search or browse system high priority performance / caching Related to performance, caching or embedded objects

Comments

@tdonohue
Copy link
Member

tdonohue commented Aug 1, 2024

Describe the bug

This is a subtopic to #3110.

As part of those performance discussions, 4Science mentioned they've found (in DSpace-CRIS) that SSR performance is improved when the Search Component (search.component.ts) is excluded from Server Side Rendering. This would mean that searching will only function for Client Side Rendering (CSR).

See the code change in https://github.com/4Science/dspace-angular/blob/main-cris/src/app/shared/search/search.component.ts#L415-L419

This should have no impact on SEO because well-behaving crawlers all use Sitemaps, and we already exclude /search and search filters in our robots.txt: https://github.com/DSpace/dspace-angular/blob/main/src/robots.txt.ejs#L11

Further testing may need to be done to ensure disabling searching in SSR doesn't have side effects for accessibility (e.g. screen readers). But, my understanding is that it should not, as these tools should all trigger CSR. This is something we can test further once a PR is created and/or offer a configuration to turn this on/off.

Assigning @atarix83 to create a PR.

Related work

Related to #3110 because it should provide benefits to SSR performance.

@tdonohue tdonohue added bug component: Discovery related to discovery search or browse system high priority performance / caching Related to performance, caching or embedded objects affects: 8.x Issue impacts 8.x releases affects: 7.x Issue impacts 7.x releases labels Aug 1, 2024
@github-project-automation github-project-automation bot moved this to 📋 To Do in DSpace 9.0 Release Aug 1, 2024
@tdonohue tdonohue moved this from 📋 To Do to 🏗 In Progress in DSpace 9.0 Release Aug 1, 2024
@paulo-graca
Copy link
Contributor

As I commented here:
#3110 (comment)

I discourage to do this, or at least, it should be configurable as well as any other part of DSpace Angular we want do disconnect from SSR.
As in the example I referred, you can have thematic repositories. If the Search Engine don't assign those keywords to your website, you will lose relevancy. Also, you have the Internet Archives (that are prepared to not necessary follow the sitemap.xml), that if they don't fetch the results, it would not be possible to persist pages in the archives.

@tdonohue
Copy link
Member Author

tdonohue commented Aug 2, 2024

I agree this should ideally be configurable. I do also want to point out that the purpose for asking for this PR to be created by 4Science is so that others can easily test it out further & report back their findings.

If we find this idea to exclude the search component from SSR is problematic, then perhaps other developers may be inspired by the PR to find a different approach.

But, if we find the PR is generally useful at helping to decrease CPU usage of SSR, then we could consider enabling by default, with an option for sites to disable it if they desire to do so.

This option might also end up being a temporary fix. If we find ways to improve Search performance in general (via #3163 and similar such tickets), then it may make sense to turn SSR back on for search results (by default).

My point is, we should be looking for several types of solutions here. This particular ticket describes a "quick fix" that may provide help to sites that are experiencing CPU spikes for SSR related to bad-behaving or highly active crawlers hitting search results. But, in the future, that "quick fix" might be removed if future performance improvements to search are able to ensure search results don't result in CPU spikes during SSR.

@tdonohue
Copy link
Member Author

@atarix83 : Would it be possible to get a PR created for this ticket in the near future? I'd like to consider including this in an 8.1 / 7.6.3 as well. Ideally, it should be configurable. It could be a simple boolean setting to turn it on/off, perhaps something like:

search:
  ssr: true/false

It can be set to false by default.

NOTE: There is also a semi-related PR to control SSR at the path level in #3682. I've recommended it be made configurable too. But, I think this search component may need it's own separate configuration because it's used in so many paths.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: 7.x Issue impacts 7.x releases affects: 8.x Issue impacts 8.x releases bug component: Discovery related to discovery search or browse system high priority performance / caching Related to performance, caching or embedded objects
Projects
Status: 🏗 In Progress
Development

No branches or pull requests

3 participants