-
Notifications
You must be signed in to change notification settings - Fork 24
[ENG-9792] Angular Universal #803
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
base: feature/pbs-25-24
Are you sure you want to change the base?
[ENG-9792] Angular Universal #803
Conversation
brianjgeiger
left a comment
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.
Overall, looks good. I had a couple of questions on some removed code.
Is it feasible to add tests to ensure that the pages that we want to render server-side will be renderable server-side? I'd hate for us to add something that breaks the functionality without realizing it.
And my understanding is that this should have no effect on client-side rendering, it would only take effect when running a server instance and we push people to that server instance. Is that correct? So we'd be safe to merge this with a full regression test from QA to make sure we didn't accidentally break anything?
| const currentResource = store.selectSnapshot(CurrentResourceSelectors.getCurrentResource); | ||
| const currentUser = store.selectSnapshot(UserSelectors.getCurrentUser); | ||
|
|
||
| if (currentResource && !id.startsWith(currentResource.id)) { | ||
| if (currentResource.type === CurrentResourceType.Projects && currentResource.parentId) { | ||
| router.navigate(['/', currentResource.parentId, 'files', id], { queryParamsHandling: 'preserve' }); | ||
| return true; | ||
| } | ||
|
|
||
| if (currentResource.type === CurrentResourceType.Preprints && currentResource.parentId) { | ||
| router.navigate(['/preprints', currentResource.parentId, id]); | ||
| return true; | ||
| } | ||
|
|
||
| if (currentResource.type === CurrentResourceType.Users) { | ||
| if (currentUser && currentUser.id === currentResource.id) { | ||
| router.navigate(['/profile']); | ||
| } else { | ||
| router.navigate(['/user', id]); | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| return currentResource.type === CurrentResourceType.Projects; | ||
| } |
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.
Is this not important? I didn't see a replacement for it anywhere, but maybe we're not really using it?
| const currentResource = store.selectSnapshot(CurrentResourceSelectors.getCurrentResource); | ||
| const currentUser = store.selectSnapshot(UserSelectors.getCurrentUser); | ||
|
|
||
| if (currentResource && !id.startsWith(currentResource.id)) { | ||
| if (currentResource.type === CurrentResourceType.Registrations && currentResource.parentId) { | ||
| router.navigate(['/', currentResource.parentId, 'files', id], { queryParamsHandling: 'preserve' }); | ||
| return true; | ||
| } | ||
|
|
||
| if (currentResource.type === CurrentResourceType.Preprints && currentResource.parentId) { | ||
| router.navigate(['/preprints', currentResource.parentId, id]); | ||
| return true; | ||
| } | ||
|
|
||
| if (currentResource.type === CurrentResourceType.Users) { | ||
| if (currentUser && currentUser.id === currentResource.id) { | ||
| router.navigate(['/profile']); | ||
| } else { | ||
| router.navigate(['/user', id]); | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| return currentResource.type === CurrentResourceType.Registrations; | ||
| } |
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.
Same question here.
Yeah, I can add unit tests for pages with SSR. Issues can occur only during build time, initial loading, or hydration (when the browser receives the SSR HTML). After that, it will run using CSR. |
Yes, that's correct. It has no effect on client-side rendering. After the browser receives the SSR HTML and hydrates, all subsequent navigation is standard client-side SPA behavior, just like a non-SSR Angular app. |
Summary of Changes