-
Notifications
You must be signed in to change notification settings - Fork 2
fix(nextjs-ssr): Enable SSR rendering tests #344
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
Draft
David Nalchevanidze (nalchevanidze)
wants to merge
21
commits into
main
Choose a base branch
from
fix-ssr
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
e56c4ae
🐞 fix(nextjs-ssr): Fix SSR rendering — always render children, use se…
nalchevanidze 15be51d
💡 refactor(nextjs-ssr): Clean up lib utilities and resolution module
nalchevanidze c6f2bc3
💡 refactor(nextjs-ssr): Remove unused Tailwind CSS setup
nalchevanidze a198006
💡 refactor(nextjs-ssr): Further cleanup of components and resolution …
nalchevanidze 0d93743
💡 refactor(nextjs-ssr): Fetch and resolve entry links lazily inside f…
nalchevanidze 8dc540a
💡 refactor(nextjs-ssr): Consolidate optimization logic into ServerOpt…
nalchevanidze a7a0679
💡 refactor(nextjs-ssr): Finalize ServerOptimization API and clean up …
nalchevanidze b373f34
✨ feat(nextjs-ssr): Pass server-side consent, identified and optimiza…
nalchevanidze 1e5922f
💡 refactor(nextjs-ssr): Consolidate ControlPanel hooks into useContro…
nalchevanidze 8d338d9
💡 refactor(nextjs-ssr): Flatten serverState and derive ControlPanelSe…
nalchevanidze c390f10
fix(nextjs-ssr): Stop using serverState.profile after SDK is ready
nalchevanidze 4514716
refactor(nextjs-ssr): Remove double entry-card wrapper in EntryCardCl…
nalchevanidze e372370
fix(nextjs-ssr): Include consent in OptimizationRoot defaults
nalchevanidze 3375600
fix(nextjs-ssr): Restore content-* and entry-text-* testids in EntryC…
nalchevanidze 38a5f6a
refactor(nextjs-ssr): Simplify EntryCard props and move isRichTextFie…
nalchevanidze 20a3dbb
test(e2e-web): Add SSR first-paint state tests and refactor shared co…
nalchevanidze 385a1bb
chore: Merge origin/main into fix-ssr
nalchevanidze 6c22250
refactor(nextjs-ssr): Extract isMergeTagNode to util and remove cast
nalchevanidze 0fd6a79
refactor(nextjs-ssr): Resolve merge tags to text nodes like Angular
nalchevanidze f4238a2
refactor(fix-ssr): Remove react-web-sdk SSR stub — moved to experimen…
nalchevanidze f32dca2
chore: Merge origin/main into fix-ssr
nalchevanidze File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file was deleted.
Oops, something went wrong.
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
29 changes: 29 additions & 0 deletions
29
implementations/nextjs-sdk_ssr/components/EntryCard.client.tsx
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| 'use client' | ||
|
|
||
| import type { ContentEntry } from '@/lib/contentful' | ||
| import { OptimizedEntry } from '@contentful/optimization-nextjs/client' | ||
| import type { JSX } from 'react' | ||
|
|
||
| interface EntryCardClientProps { | ||
| entry: ContentEntry | ||
| liveUpdates?: boolean | ||
| testId?: string | ||
| } | ||
|
|
||
| export function EntryCardClient({ entry, liveUpdates, testId }: EntryCardClientProps): JSX.Element { | ||
| return ( | ||
| <OptimizedEntry baselineEntry={entry} liveUpdates={liveUpdates}> | ||
| {({ fields, sys: { id } }) => ( | ||
| <div data-test-entry-id={id} data-testid={`content-${testId}`}> | ||
| <div | ||
| aria-label={`${fields.text ?? ''} [Entry: ${id}]`} | ||
| data-testid={`entry-text-${testId}`} | ||
| > | ||
| <p>{String(fields.text ?? '')}</p> | ||
| <p>{`[Entry: ${id}]`}</p> | ||
| </div> | ||
| </div> | ||
| )} | ||
| </OptimizedEntry> | ||
| ) | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
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.
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.
What was the reason you deleted this? Do you know what its purpose is?
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.
not sure. was it probably not used? fist should check what it does.
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.
This component is required to pass state from the SSR context to the client-side context, and it must be a component because client hooks are not allowed in server components. Please try to understand what you are doing in the course of "fixing" an issue.
Uh oh!
There was an error while loading. Please reload this page.
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.
first of all, it was excluded by
claudewhile fixinge2etests and not having it did not broke anything. app functioned as it is(if it desired behavior that is required, lets adde2etest for it, and what it does).after manual investigation: passing state is done by
<OptimizationRoot defauls={}>andNextjsOptimizationStateis just component to call hookhydrateOptimizationData->await getRequiredBridge(sdk).hydrateOptimizationData(data). interesting is what does it and why? why its not provider wraping whole app? and do we really need it if it does not affect apps functionality? and just naming does not give enough clarity to understand its necessity. if it ware provider to provide backend context. name would make perfect sence but then questions is why we have two places for that :OptimizationRoot.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.
Always happy to collaborate and discuss — I'd appreciate keeping the feedback constructive though.
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.
Charles Hudson (@phobetron) David Nalchevanidze (@nalchevanidze) How can we make sure that we do not rely on knowing everything upfront? One would expect a test or a the compiler to let you know when you have broke something. If this is not needed for today's use case, my instinct would be to get rid of it.
Concretely, we need to invest in adding automated tests that ensure key functionality is not unintentionally changed. Claude will cover more ground than us as humans, and we need the safeguards to ensure we catch changes like that.