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.

Sign up for GitHub

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

Jump to bottom

docs(headless commerce SSR): basics for commerce SSR + Remix sample #4708

Open
wants to merge 42 commits into
base: master
Choose a base branch
from
Open

docs(headless commerce SSR): basics for commerce SSR + Remix sample #4708

wants to merge 42 commits into from

Conversation

fbeaudoincoveo
Copy link
Contributor

y-lakhdar and others added 30 commits October 31, 2024 16:17
@y-lakhdar
recs server-side working
187fc0b
@y-lakhdar
recognize search action promises
d571439
@y-lakhdar
add typeguard
4dc2717
@y-lakhdar
prevent multiple recommendations with the same slot
2e5f794
@y-lakhdar
clean
de43049
@y-lakhdar
draft
9254b93
@y-lakhdar
working draft
f2d1fa1
@y-lakhdar
test
bc63a92
@y-lakhdar
Merge branch 'master' of github.com:coveo/ui-kit into ssr-recs
8d170d2
@y-lakhdar
update sample
e60bfe5
@y-lakhdar
recommendation hydration working
be888c8
@y-lakhdar
clean build function
c4ff2b9
@y-lakhdar
clean exports
2783740
@y-lakhdar
filter invalid and duplicate recommendations
8a6f9d8
@y-lakhdar
refacto
025b34f
@y-lakhdar
simplify build factory
07d1ec8
@y-lakhdar
adjust factory params and throw error on bad engine definition
9b049fd
@y-lakhdar
do not ask props for disabled controllers
84e5c53
@y-lakhdar
remove comments
018c0be
@y-lakhdar
update warning message
a7200d8
@y-lakhdar
fix export
785951f
@y-lakhdar
clean PR
f342da5
@y-lakhdar
clean controller build condition
677fd45
@y-lakhdar
create commerce build result type
87d0792
@y-lakhdar
remove unnecessary error
16c2b5e
@y-lakhdar
revert EngineDefinitionControllersPropsOption
dbffb51
@y-lakhdar
update UT
961c28e
@alexprudhomme
Add recs to sample
c58b3ae
@alexprudhomme
Merge branch 'master' into ssr-recs
81ba114
@alexprudhomme
no core engine
b8e6e16
@github-actions GitHub Actions
Copy link

Pull Request Report

PR Title

✅ Title follows the conventional commit spec.

Live demo links

Bundle Size

File Old (kb) New (kb) Change (%)
case-assist 241.4 241.4 0
commerce 346 346 0
search 413 413 0
insight 403.8 403.8 0
recommendation 253.6 253.6 0
ssr 406.5 406.5 0
ssr-commerce 361.1 361.1 0

SSR Progress

Use case SSR (#) CSR (#) Progress (%)
search 39 44 89
recommendation 0 4 0
case-assist 0 6 0
insight 0 27 0
commerce 0 15 0
Detailed logs search : buildInteractiveResult
search : buildInteractiveInstantResult
search : buildInteractiveRecentResult
search : buildInteractiveCitation
search : buildGeneratedAnswer
recommendation : missing SSR support
case-assist : missing SSR support
insight : missing SSR support
commerce : missing SSR support

fbeaudoincoveo
fbeaudoincoveo commented Nov 25, 2024
View reviewed changes
packages/samples/headless-commerce-ssr-remix/app/components/product-view.tsx
uniqueId: productId,
} = catalogItem;

const viewed = useRef(false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To log the view event only once

fbeaudoincoveo
fbeaudoincoveo commented Nov 25, 2024
View reviewed changes
packages/samples/headless-commerce-ssr-remix/app/routes/listings.$listingId.tsx Outdated
navigatorContext={navigatorContext}
>
<h2>
{params.listingId
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Side note: I find it kind of annoying that there's no way to set a display name for PLPs in the CMH. It feels silly to have to do something like that to set the page title.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be in CMH?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CC @Spuffynism

fbeaudoincoveo
fbeaudoincoveo commented Nov 25, 2024
View reviewed changes
packages/samples/headless-commerce-ssr-remix/client/external-catalog-api.ts
@@ -0,0 +1,36 @@
export type ExternalCatalogItem = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The implementation of this "API" is completely silly, but the idea is to show clearly that the main product information shown on a PDP should typically come from some external service, not from a Coveo query.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not in the current next sample and I don't think it is necessary imo. This is different from the cart since it has 0 relation to coveo except for it to potentially use the same productId. I think we should remove this.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can keep it but can we add some comments ? Similar to here ?
https://github.com/coveo/ui-kit/blob/master/packages/samples/headless-ssr-commerce/actions/external-cart-api.ts

louis-bompart
louis-bompart approved these changes Nov 25, 2024
View reviewed changes
Copy link
Collaborator

@louis-bompart louis-bompart left a comment

Choose a reason for hiding this comment

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

LGTM, some nitpik

packages/samples/headless-commerce-ssr-remix/app/entry.server.tsx Outdated Show resolved Hide resolved
packages/samples/headless-commerce-ssr-remix/app/root.tsx Outdated Show resolved Hide resolved
packages/samples/headless-commerce-ssr-remix/app/routes/listings.$listingId.tsx Show resolved Hide resolved
packages/samples/headless-commerce-ssr-remix/app/routes/listings.$listingId.tsx Outdated
navigatorContext={navigatorContext}
>
<h2>
{params.listingId
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be in CMH?

packages/samples/headless-commerce-ssr-remix/app/routes/listings.$listingId.tsx Outdated Show resolved Hide resolved
packages/samples/headless-commerce-ssr-remix/package.json Outdated Show resolved Hide resolved
alexprudhomme
alexprudhomme reviewed Nov 25, 2024
View reviewed changes
Copy link
Contributor

@alexprudhomme alexprudhomme left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I am bringing a lot of changes to the next sample in my PRs so we need to keep this mind and make sure they are perfectly aligned
#4709

packages/samples/headless-commerce-ssr-remix/package.json
@@ -0,0 +1,34 @@
{
"name": "headless-commerce-ssr-remix",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"name": "headless-commerce-ssr-remix",
"name": "@coveo/headless-commerce-ssr-remix",

packages/samples/headless-commerce-ssr-remix/lib/commerce-engine-config.ts
standaloneSearchBox: defineStandaloneSearchBox({
options: {redirectionUrl: '/search'},
}),
instantProducts: defineInstantProducts({options: {}}),
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove the props here once this gets merged. It was a mistake in the implementation. props for instantProducts should not be mandatary.
#4700

alexprudhomme
alexprudhomme approved these changes Nov 25, 2024
View reviewed changes
packages/samples/headless-commerce-ssr-remix/client/external-catalog-api.ts
@@ -0,0 +1,36 @@
export type ExternalCatalogItem = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not in the current next sample and I don't think it is necessary imo. This is different from the cart since it has 0 relation to coveo except for it to potentially use the same productId. I think we should remove this.

packages/samples/headless-commerce-ssr-remix/app/routes/listings.$listingId.tsx Show resolved Hide resolved
packages/samples/headless-commerce-ssr-remix/projects.json
@@ -0,0 +1,17 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

isnt this supposed to be "project.json" ? and not "projects.json". Not sure if it matters

Base automatically changed from ssr-recs to master November 26, 2024 20:15
@github-actions github-actions bot requested a review from a team as a code owner November 26, 2024 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexprudhomme alexprudhomme alexprudhomme approved these changes

@louis-bompart louis-bompart louis-bompart approved these changes

@olamothe olamothe Awaiting requested review from olamothe olamothe is a code owner automatically assigned from coveo/search

@fpbrault fpbrault Awaiting requested review from fpbrault fpbrault is a code owner automatically assigned from coveo/search

@y-lakhdar y-lakhdar Awaiting requested review from y-lakhdar y-lakhdar is a code owner automatically assigned from coveo/search

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fbeaudoincoveo @louis-bompart @alexprudhomme @y-lakhdar