Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Setup root error boundary to gracefully handle all unhandled errors #104
Setup root error boundary to gracefully handle all unhandled errors #104
Changes from 3 commits
d76aaf6
433ef3d
4243abb
3b87d2f
49900c8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Could you explain the necessity of this please at the page.tsx level?
At this level I don't foresee us fetching anything that we don't want cached such as a list of commodity names that don't really change.
From what I understand NextJS will automatically switch these pages to dynamically rendered if uncached data is detected. It seems to go against the premise of caching if we were to force these routes to be dynamic.
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 had removed it in one of the commits in this PR based on the same understanding that it should not make these fetch calls during build but for some reason it was still making these calls during the github actions build. Have added this for now to fix that issue until I look into what's causing this to make these calls. However, it is needed in the other server-error scenario because I am not doing anything there for nextJs to detect that it should be done dynamically.
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 also found this useful implementation to simulate github actions when needed, more details on the purpose and intent - #112. Will implement this to help with debugging these issues.
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.
The calls that populate cached data at the page.tsx level should happen at build (atm we only do this to fetch an updated list of possible commodities). The data is required by the children. But you're probably talking specifically about the playground route, where you're right...though it's an exception to use 'no-cache' in our app.
From what I'm understanding here, there's a build error w/o this dynamic flag that occurs in the playground route because the server fetches are set to no-cache.
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.
Updated this to use cache config.
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.
It still fails - https://github.com/ed-pilots-network/frontend/actions/runs/6192011962/job/16811315676. I will debug this once I setup
act
(#112) for our package so I can do this locally. For now, have reverted back to forcing dynamic rendering.