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.

Sign up for GitHub

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

Jump to bottom

[Issue #2622] Update opp page for ISR #2724

Merged
merged 6 commits into from
Nov 12, 2024
Merged

[Issue #2622] Update opp page for ISR #2724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
12aab7f
Update opp page for ISR
acouch Nov 4, 2024
08cd014
Merge branch 'main' into acouch/opp-page-isr
acouch Nov 7, 2024
bfe025c
Update opp page.tsx to reduce cache lifetime
acouch Nov 8, 2024
fca9dc2
Update ci-frontend-a11y.yml
acouch Nov 8, 2024
060650e
Add e2e test
acouch Nov 8, 2024
d6eed6e
Remove file
acouch Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions .github/workflows/ci-frontend-a11y.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,6 @@ jobs:
- name: Create screenshots directory
run: mkdir -p screenshots-output

- name: Build project
run: npm run build

- name: Start server and log output
run: npm run start &

- name: Start API Server for search results
run: |
cd ../api
Expand All @@ -52,6 +46,14 @@ jobs:
../api/bin/wait-for-api.sh
shell: bash

- name: Build Site
run: |
cat .env.development >> .env.local
npm run build

- name: Run Server
run: npm run start &

- name: Wait for frontend to be ready
run: |
# Ensure the server wait script is executable
Expand Down
15 changes: 11 additions & 4 deletions frontend/src/app/[locale]/opportunity/[id]/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import NotFound from "src/app/[locale]/not-found";
import fetchers from "src/app/api/Fetchers";
import { OPPORTUNITY_CRUMBS } from "src/constants/breadcrumbs";
import { ApiRequestError, parseErrorStatus } from "src/errors";
import withFeatureFlag from "src/hoc/search/withFeatureFlag";
import { Opportunity } from "src/types/opportunity/opportunityResponseTypes";

import { getTranslations } from "next-intl/server";
Expand All @@ -20,6 +19,8 @@ import OpportunityIntro from "src/components/opportunity/OpportunityIntro";
import OpportunityLink from "src/components/opportunity/OpportunityLink";
import OpportunityStatusWidget from "src/components/opportunity/OpportunityStatusWidget";

export const revalidate = 600; // invalidate ten minutes

export async function generateMetadata({ params }: { params: { id: string } }) {
const t = await getTranslations({ locale: "en" });
const id = Number(params.id);
Expand All @@ -41,6 +42,10 @@ export async function generateMetadata({ params }: { params: { id: string } }) {
return meta;
}

export function generateStaticParams() {
return [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok so this seems to be the configuration that this change sets up:

  • no pages are built and pre-cached at build time
  • all pages are rendered the on their first hit within a 10 minute window, then cached for 10 minutes

Is that right? Is the idea to try this out and evaluate how it works, or are there plans for next steps, such as as generating a list of all current opportunities at build time and pre-caching? Is the 10 minute window a ballpark estimate at this point that we will analyze in some way after launch and adjust, or is that based on data that leads us to believe it should be a pretty solid permanent setting?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is that right? Is the idea to try this out and evaluate how it works, or are there plans for next steps, such as as generating a list of all current opportunities at build time and pre-caching?

Yes. We could certainly to that in the future. We don't have a good way of getting all of the opp ids. We might want an API for that in the future.

Is the 10 minute window a ballpark estimate at this point that we will analyze in some way after launch and adjust, or is that based on data that leads us to believe it should be a pretty solid permanent setting?

The file links refresh every 30 mins, so that is the higher end. The 10 mins is something we can test and adjust in the future.

}

function emptySummary() {
return {
additional_info_url: null,
Expand Down Expand Up @@ -77,7 +82,11 @@ function emptySummary() {
};
}

async function OpportunityListing({ params }: { params: { id: string } }) {
export default async function OpportunityListing({
params,
}: {
params: { id: string };
}) {
const id = Number(params.id);
const breadcrumbs = Object.assign([], OPPORTUNITY_CRUMBS);
// Opportunity id needs to be a number greater than 1
Expand Down Expand Up @@ -127,5 +136,3 @@ async function OpportunityListing({ params }: { params: { id: string } }) {
</div>
);
}

export default withFeatureFlag(OpportunityListing, "showSearchV0");
Copy link
Collaborator

Choose a reason for hiding this comment

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

when we merge, worth noting in the commit that this change is also included in case anyone wants to track it down later

18 changes: 18 additions & 0 deletions frontend/tests/e2e/opportunity.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/* eslint-disable testing-library/prefer-screen-queries */
import { expect, test } from "@playwright/test";

test.beforeEach(async ({ page }) => {
await page.goto("/opportunity/1");
});

test.afterEach(async ({ context }) => {
await context.close();
});

test("has title", async ({ page }) => {
await expect(page).toHaveTitle(/^Opportunity Listing - */);
});

test("has page attributes", async ({ page }) => {
await expect(page.getByText("Forecasted")).toBeVisible();
});