Standardize extraction of account id and page id from route #456
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
area/apps > hash*
github-actions
bot
added
playwright
and removed
area/deps
kachkaev
changed the title
kachkaev
commented
Apr 4, 2022
| @@ -16,7 +16,7 @@ import { useCreatePage } from "../../../hooks/useCreatePage"; | |||
|
|
|||
| type AccountPageListProps = { | |||
| accountId: string; | |||
| currentPageEntityId: string; | |||
| currentPageEntityId?: string; | |||
There was a problem hiding this comment.
@teenoh I’ve made this parameter optional so that we can see the list of pages even if we browse a type. This behavior feels more logical to me, but I’m happy to change it back if the spec suggests otherwise.
kachkaev
commented
Apr 4, 2022
Comment on lines
+41
to
+44
| export interface UseRouteAccountInfo { | ||
| (options: { allowUndefined: true }): RouteAccountInfo | undefined; | ||
| (options?: { allowUndefined?: false }): RouteAccountInfo; | ||
| } |
There was a problem hiding this comment.
@teenoh this gives us extra type safety when we use the hook. We don’t need ! as discussed in #460 (comment)
kachkaev
commented
Apr 4, 2022
| @@ -2,7 +2,7 @@ import { test, expect } from "@playwright/test"; | |||
|
|
|||
| test("guest user navigation to login and signup pages", async ({ page }) => { | |||
| await page.goto("/"); | |||
| await page.waitForNavigation({ url: "**/login" }); | |||
| await page.waitForURL("**/login"); | |||
There was a problem hiding this comment.
waitForNavigation failed for me in CI, so I investigated it and found the fix. Unlike waitForNavigation, waitForURL succeeds even if we are on the right page already. Not sure if I have fixed some general test flakiness by this or this was a new glitch. It might be related to a change in _app.page.tsx.
kachkaev
changed the title
teenoh
approved these changes
Apr 5, 2022
vilkinsons
added
type/eng > frontend
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
🌟 What is the purpose of this PR?
This PR provides an alternative solution to #456. It prevents account id from being undefined during SSR by not rendering app pages on the server. Frontend page files are converted to
kebab-casegiven the opportunity.🔍 What does this change?
New hooks:
useRouteAccountInfoanduseRoutePageInfo(inspired byCurrentWorkspaceContext)Replacement of all
router.query["pageId|accountId"]with new hooks. This improves type-safety and provides future-proofing. Using slugs instead of ids should be easier now.Page files were renamed
[account-slug]and[page-slug]instead of[accountId]and[pageId]_app.page.tsis updated: server-side rendering is skipped because page and account ids are undefined and this may crash some componentsApp UI often depends on
[account-slug]and other query params. However,router.queryis empty during server-side rendering for pages that don’t usegetServerSideProps. By showing app skeleton on the server, we avoid UI mismatches during rehydrationkebab-caseis enforced for"[frontend]/src/pages/**/*"to speed-up future reviews. Related to Configure ESLint rules for shared folders, bump ESLint deps #453null. We should replace it with Hash logo or some loader. Alternatively, we can consider making all/[account-slug]/*pages server-rendered by addinggetServerSideProps. However, this will significantly increase server load and create some additional edge cases for UI layouts.🐾 Next steps
Continue working on UI components for the entity editor
Consider parsing
[account-slug]and converting it toaccountIdinsideRouteAccountInfoProvider🔗 Related links
🛡 What tests cover this?
❓ How to test this?