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

1780/Cleanup getStaticProps and getStaticPaths #1861

Merged
merged 8 commits into from
Sep 29, 2021
Merged

Conversation

seanmalbert
Copy link
Collaborator

@seanmalbert seanmalbert commented Sep 21, 2021

Issue

Addresses #1780

  • This change addresses the issue in full
  • This change addresses only certain aspects of the issue
  • This change is a dependency for another issue
  • This change has a dependency from another issue

Description

The cache revalidation piece doesn't actually change here, the main changes are is so that errors aren't swallowed on builds and fails them (should cover #1618) and I removed the export script, since it isn't valid with fallback: true. So we'll have to change the build command to replace export with start.

This doesn't address the second part of #1780, which is to add applicationOpenDate to the form and check against that before displaying, since that feels separate enough to warrant another PR.

Type of change

  • 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)
  • Prototype/POC (not to merge)
  • This change is a refactor/address technical debt
  • This change requires a documentation update
  • This change requires a SQL Script

How Can This Be Tested/Reviewed?

I tested locally by starting the back end, then building and starting public. If you have partners started, you should be able to add/edit a listing and after CACHE_REVALIDATE (set in your .env) expires, you should see the update after refreshing. You should also be able to stop the backend and continue to see listings and go to detail pages.

  • Desktop View
  • Mobile View
  • Test A
  • Test B

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have reviewed the changes in a desktop view
  • I have reviewed the changes in a mobile view
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have assigned reviewers
  • I have updated the changelog to include a description of my changes
  • I have run yarn generate:client if I made backend changes

The main piece is to remove the export script, since it doesn't work with fallback: true (https://nextjs.org/docs/advanced-features/static-html-export#caveats)
@netlify
Copy link

netlify bot commented Sep 21, 2021

✔️ Deploy Preview for dev-partners-bloom ready!

🔨 Explore the source changes: c1f2072

🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-partners-bloom/deploys/6154c9e2fd8e0700078ca619

😎 Browse the preview: https://deploy-preview-1861--dev-partners-bloom.netlify.app

@netlify
Copy link

netlify bot commented Sep 21, 2021

✔️ Deploy Preview for dev-bloom ready!

🔨 Explore the source changes: c1f2072

🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-bloom/deploys/6154c9e24998c90008f758d6

😎 Browse the preview: https://deploy-preview-1861--dev-bloom.netlify.app

@netlify
Copy link

netlify bot commented Sep 21, 2021

✔️ Deploy Preview for dev-storybook-bloom ready!

🔨 Explore the source changes: c1f2072

🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-storybook-bloom/deploys/6154c9e28fff2a000734e844

😎 Browse the preview: https://deploy-preview-1861--dev-storybook-bloom.netlify.app

Comment on lines 66 to 73
<Hero
title={heroTitle}
buttonTitle={t("welcome.seeRentalListings")}
buttonLink="/listings"
allApplicationsClosed={
!props.listings.some(listingOpen) && !props.listings.some(openDateState)
}
/>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This piece has been updated on Alameda for some time now, since the current logic doesn't account for listings with no application due date. I'm not sure we really need this check here, and it gives us one less route where we're querying listings.

CHANGELOG.md Outdated
@@ -46,6 +46,7 @@ All notable changes to this project will be documented in this file. The format

- Upgrade the public and partners sites to Next v11 and React v17 ([#1793](https://github.com/bloom-housing/bloom/pull/1793)) (Jared White)
- **Breaking Change**
- The cache revalidation piece doesn't actually change here, the main changes are is so that errors aren't swallowed on builds and fails them (should cover #1618) and I removed the export script, since it isn't valid with [fallback: true](https://nextjs.org/docs/advanced-features/static-html-export#caveats). So we'll have to change the build command to replace `export` with `start`. ([#1861](https://github.com/bloom-housing/bloom/pull/1861))
Copy link
Collaborator

Choose a reason for hiding this comment

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

are is so

@emilyjablonski
Copy link
Collaborator

I'm a little confused as to why this one is titled cache revalidation / what is changing there? Maybe just the title needs to be changed?

@seanmalbert seanmalbert changed the title 1780/Cache revalidate 1780/Cleanup getStaticProps and getStaticPaths Sep 23, 2021
@seanmalbert
Copy link
Collaborator Author

I'm a little confused as to why this one is titled cache revalidation / what is changing there? Maybe just the title needs to be changed?

Updated title to be more appropriate for what changed.

@seanmalbert
Copy link
Collaborator Author

@emilyjablonski , updated change log and changed PR title.

@emilyjablonski
Copy link
Collaborator

LGTM apart from the build issues, not sure if they were going to be resolved pre- or post-merge

@seanmalbert
Copy link
Collaborator Author

LGTM apart from the build issues, not sure if they were going to be resolved pre- or post-merge

So the reason why the public build fails on this branch is because the build actually depends on the backend server. If you look at passing public builds, you'll see errors that are swallowed by the try catches. So I'll need to think about the best way to handle this that doesn't require adding the catch back in.

@seanmalbert seanmalbert merged commit f3dc940 into dev Sep 29, 2021
seanmalbert added a commit to CityOfDetroit/bloom that referenced this pull request Jun 23, 2022
* updates to allow for errors to stop builds

The main piece is to remove the export script, since it doesn't work with fallback: true (https://nextjs.org/docs/advanced-features/static-html-export#caveats)

* Update CHANGELOG.md

* Update CHANGELOG.md

* updates getStaticPaths to not error in test env

* updates getStaticProps to not error in test env

* fix: testing

* fix: updated listings and id/slug and builds locally at least
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.

None yet

2 participants