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

fix(react|redux): commerce pages in ssr not working #960

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(react|redux): commerce pages in ssr not working #960

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/react/src/contents/hooks/__tests__/useCommercePages.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
mockCommercePagesInitialState,
mockCommercePagesLoadingState,
mockCommercePagesState,
slug,
} from 'tests/__fixtures__/contents/index.mjs';
import { fetchCommercePages } from '@farfetch/blackout-redux';
import { withStore } from '../../../../tests/helpers/index.js';
Expand Down Expand Up @@ -84,6 +85,7 @@ describe('useCommercePages', () => {

expect(fetchCommercePages).toHaveBeenCalledWith(
{
slug,
brand: commercePageQuery.brand,
category: commercePageQuery.category,
contentTypeCode: commercePageQuery.contentTypeCode,
Expand Down Expand Up @@ -124,6 +126,7 @@ describe('useCommercePages', () => {

expect(fetchCommercePages).toHaveBeenCalledWith(
{
slug,
brand: commercePageQuery.brand,
category: commercePageQuery.category,
contentTypeCode: commercePageQuery.contentTypeCode,
Expand Down
29 changes: 29 additions & 0 deletions packages/react/src/contents/hooks/__tests__/useContentPage.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { cleanup, renderHook } from '@testing-library/react';
import { ContentPageType } from '@farfetch/blackout-client';
import { fetchContentPage } from '@farfetch/blackout-redux';
import {
mockCommercePageWithDataState,
mockContentPageEntry,
mockContentPageErrorState,
mockContentPageInitialState,
Expand Down Expand Up @@ -102,6 +103,34 @@ describe('useContentPage', () => {

expect(fetchContentPage).not.toHaveBeenCalled();
});

it('should return values correctly with commercePagesHash option', () => {
const { result } = renderHook(
() =>
useContentPage(
ContentPageType.Listing,
{ slug },
{ isCommercePage: true },
),
{
wrapper: withStore(mockCommercePageWithDataState),
},
);

expect(result.current).toStrictEqual({
data: [
mockCommercePageWithDataState.entities.contents[
mockContentPageEntry.publicationId
],
],
isLoading: false,
error: null,
isFetched: true,
actions: {
fetch: expect.any(Function),
},
});
});
});

describe('actions', () => {
Expand Down
1 change: 1 addition & 0 deletions packages/react/src/contents/hooks/types/useCommercePages.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { CommercePagesRankingStrategy } from '@farfetch/blackout-redux';
import type { Config, QueryCommercePages } from '@farfetch/blackout-client';

export interface UseCommercePagesOptions extends QueryCommercePages {
slug: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be a breaking change as well, can't it be at least defined as optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned before, this is only used by BG and this will fix the commerce pages in this package version

enableAutoFetch?: boolean;
strategy?: CommercePagesRankingStrategy;
fetchConfig?: Config;
Expand Down
1 change: 1 addition & 0 deletions packages/react/src/contents/hooks/types/useContentPage.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@ import type { Config } from '@farfetch/blackout-client';
export interface UseContentPageOptions {
enableAutoFetch?: boolean;
fetchConfig?: Config;
isCommercePage?: boolean;
}
2 changes: 1 addition & 1 deletion packages/react/src/contents/hooks/useCommercePages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ const useCommercePages = <T = ComponentType[]>(
const query = useMemo(
() => ({
contentTypeCode: ContentTypeCode.CommercePages,
...fetchQuery,
codes: fetchQuery.slug,
}),
[fetchQuery],
);
Expand Down
14 changes: 10 additions & 4 deletions packages/react/src/contents/hooks/useContentPage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,20 @@ const useContentPage = <T = [ComponentType]>(
) => {
const store = useStore();

const { enableAutoFetch = true, fetchConfig } = options;
const {
enableAutoFetch = true,
fetchConfig,
isCommercePage = false,
} = options;

const query = useMemo(
() => ({
contentTypeCode: ContentTypeCode.ContentPage,
codes: fetchQuery.slug.split('?')[0] as string,
contentTypeCode: isCommercePage
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not understand this approach. Why would the user need to set isCommercePage to true if there is a useCommercePages hook for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, to distinguish from the fetch to commerce pages endpoint directly we use the contentType commercePages and for the request to Serverless API, we use contentType contentPages, to generate the hash

Copy link
Contributor

@boliveira boliveira Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, but in that case, why not use the useCommercePages hook by itself? I think that this signature is leaking implementation details that the consumer should not have to worry about. In my opinion, the correct solution would be for the serverInitialState to generate entries for both contentTypes in the store so the consumer does not need to worry about this.
What I mean is, the serverInitialState would set an entry for the commercePages and contentPages contentTypes containing the same data. I think they have the same data format so it would work. If that assumption is not correct, then we should adapt the responses from FO to the proper format.

? ContentTypeCode.CommercePages
: ContentTypeCode.ContentPage,
codes: fetchQuery.slug,
}),
[fetchQuery.slug],
[fetchQuery.slug, isCommercePage],
);

const fetchQueryWithoutSlug = useMemo(() => {
Expand Down
11 changes: 6 additions & 5 deletions packages/redux/src/contents/actions/__tests__/fetchCommercePages.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import * as actionTypes from '../../actionTypes.js';
import * as normalizr from 'normalizr';
import {
commercePagesQuery,
commercePageQuery,
commercePageQueryWithoutSlug,
expectedCommercePagesNormalizedPayload,
mockCommercePages,
} from 'tests/__fixtures__/contents/index.mjs';
Expand Down Expand Up @@ -44,12 +45,12 @@ describe('fetchCommercePages() action creator', () => {
(getCommercePages as jest.Mock).mockRejectedValueOnce(expectedError);

await expect(
async () => await fetchCommercePages(commercePagesQuery)(store.dispatch),
async () => await fetchCommercePages(commercePageQuery)(store.dispatch),
).rejects.toThrow(expectedError);

expect(getCommercePages).toHaveBeenCalledTimes(1);
expect(getCommercePages).toHaveBeenCalledWith(
commercePagesQuery,
commercePageQueryWithoutSlug,
expectedConfig,
);
expect(store.getActions()).toEqual([
Expand All @@ -72,7 +73,7 @@ describe('fetchCommercePages() action creator', () => {
it('should create the correct actions for when the get commerce pages procedure is successful', async () => {
(getCommercePages as jest.Mock).mockResolvedValueOnce(mockCommercePages);

await fetchCommercePages(commercePagesQuery)(store.dispatch).then(
await fetchCommercePages(commercePageQuery)(store.dispatch).then(
clientResult => {
expect(clientResult).toBe(mockCommercePages);
},
Expand All @@ -83,7 +84,7 @@ describe('fetchCommercePages() action creator', () => {
expect(normalizeSpy).toHaveBeenCalledTimes(1);
expect(getCommercePages).toHaveBeenCalledTimes(1);
expect(getCommercePages).toHaveBeenCalledWith(
commercePagesQuery,
commercePageQueryWithoutSlug,
expectedConfig,
);
expect(actionResults).toMatchObject([
Expand Down
10 changes: 6 additions & 4 deletions packages/redux/src/contents/actions/factories/fetchCommercePagesFactory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@ import {
type CommercePages,
type Config,
type GetCommercePages,
type QueryCommercePages,
toBlackoutError,
} from '@farfetch/blackout-client';
import {
type CommercePagesRankingStrategy,
ContentTypeCode,
type FetchCommercePagesAction,
type QueryCommercePagesWithSlug,
} from '../../types/index.js';
import { contentEntries } from '../../../entities/schemas/content.js';
import { normalize } from 'normalizr';
Expand All @@ -29,7 +29,7 @@ import type { Dispatch } from 'redux';
const fetchCommercePagesFactory =
(getCommercePages: GetCommercePages) =>
(
query: QueryCommercePages,
query: QueryCommercePagesWithSlug,
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be a breaking change, can't you calculate a default value for the slug or at least make it optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only BG is using the commerce pages of this package version. We can change this an talk to them to implement this

Copy link
Contributor

Choose a reason for hiding this comment

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

Our boilerplate is also making use of this, so we would have to update it as well.

strategy?: CommercePagesRankingStrategy,
config?: Config,
) =>
Expand All @@ -39,17 +39,19 @@ const fetchCommercePagesFactory =
let hash: string | undefined;

try {
const { slug, ...queryWithoutSlug } = query;

hash = generateContentHash({
contentTypeCode: ContentTypeCode.CommercePages,
...query,
codes: slug,
Copy link
Contributor

Choose a reason for hiding this comment

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

This will make requests for commerce pages containing different query parameters to be indexed on the same entry in the redux store. Is this ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only generate an hash differently. The hash for commerce pages need to be the url instead of query params

});

dispatch({
payload: { hash },
type: actionTypes.FETCH_COMMERCE_PAGES_REQUEST,
});

const result = await getCommercePages(query, config);
const result = await getCommercePages(queryWithoutSlug, config);
const rankedResult = applyCommercePagesRankingStrategy(result, strategy);

dispatch({
Expand Down
38 changes: 24 additions & 14 deletions packages/redux/src/contents/serverInitialState.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
import {
applyCommercePagesRankingStrategy,
generateContentHash,
} from './utils.js';
import { buildQueryStringFromObject } from '../helpers/index.js';
import { contentEntries } from '../entities/schemas/content.js';
import { generateContentHash } from './utils.js';
import { type ContentsState } from './types/index.js';
import { get, merge } from 'lodash-es';
import { INITIAL_STATE_CONTENT } from './reducer.js';
import { normalize } from 'normalizr';
import parse from 'url-parse';
import type { ContentsState } from './types/index.js';
import type { ServerInitialState } from '../types/serverInitialState.types.js';

/**
Expand All @@ -15,28 +18,36 @@ import type { ServerInitialState } from '../types/serverInitialState.types.js';
*
* @returns Initial state for the contents reducer.
*/
const serverInitialState: ServerInitialState = ({ model }) => {
const serverInitialState: ServerInitialState = ({ model, strategy }) => {
if (!get(model, 'searchContentRequests')) {
return { contents: INITIAL_STATE_CONTENT };
}

const { searchContentRequests, slug, seoMetadata, subfolder } = model;
const url = subfolder !== '/' ? slug?.replace(subfolder, '') : slug;
const normalizedUrl = url
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? If json=true is passed, the response returned by FO will not be html, so the application will not be run in this case.

?.replace('?json=true', '')
.replace('&json=true', '');

const contents = searchContentRequests.reduce((acc, item) => {
const { searchResponse } = item;
const firstSearchResponseItem = searchResponse.entries[0];

if (!firstSearchResponseItem) {
return acc;
}

const {
searchResponse,
filters: { codes, contentTypeCode },
} = item;
let response = searchResponse;
const isCommercePage = contentTypeCode === 'commerce_pages';
const code = isCommercePage ? normalizedUrl : codes?.[0];
const hash = generateContentHash({
codes: firstSearchResponseItem.code,
contentTypeCode: firstSearchResponseItem.contentTypeCode,
codes: code,
contentTypeCode: contentTypeCode,
});

if (isCommercePage) {
response = applyCommercePagesRankingStrategy(searchResponse, strategy);
richard190m marked this conversation as resolved.
Show resolved Hide resolved
}

const { entities, result } = {
...normalize({ hash, ...searchResponse }, contentEntries),
...normalize({ hash, ...response }, contentEntries),
};

return merge(acc, {
Expand All @@ -53,7 +64,6 @@ const serverInitialState: ServerInitialState = ({ model }) => {
});
}, {});

const url = subfolder !== '/' ? slug?.replace(subfolder, '') : slug;
const { pathname, query } = parse(url, true);

delete query.json;
Expand Down
5 changes: 5 additions & 0 deletions packages/redux/src/contents/types/actions.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type {
BlackoutError,
Contents,
ContentType,
QueryCommercePages,
SEOMetadata,
} from '@farfetch/blackout-client';
import type { ContentEntity } from '../../entities/index.js';
Expand All @@ -22,6 +23,10 @@ export type ContentsPayload = NormalizedSchema<
ContentsNormalized
> & { hash: Hash };

export interface QueryCommercePagesWithSlug extends QueryCommercePages {
slug: string;
}

/**
* Fetch Content Page Action
*/
Expand Down
2 changes: 2 additions & 0 deletions packages/redux/src/types/serverInitialState.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import type { CommercePagesRankingStrategy } from '../contents/types/commercePagesRankingStrategy.types.js';
import type { LocaleState } from '../locale/index.js';
import type { Model } from './model.types.js';
import type { ProductsState } from '../products/index.js';
import type { StoreState } from './storeState.types.js';

export type ServerInitialState = (data: {
model: Model;
strategy?: CommercePagesRankingStrategy;
options?: { productImgQueryParam?: string };
}) => Omit<StoreState, 'products' | 'locale'> & {
products?: Partial<ProductsState>;
Expand Down
20 changes: 11 additions & 9 deletions tests/__fixtures__/contents/commercePages.fixtures.mts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,26 +9,28 @@ import {
generateContentHash,
} from '@farfetch/blackout-redux';

export const commercePagesQuery = {
type: CommercePagesType.Listing,
gender: 0,
brand: 5030844,
category: '136643',
};

export const slug = 'woman/gucci';
export const commercePageQuery = {

export const commercePageQueryWithoutSlug = {
brand: 5030844,
category: '136643',
type: CommercePagesType.Listing,
gender: GenderCode.Woman,
contentTypeCode: 'commerce_pages',
};

export const commercePageQuery = {
slug,
...commercePageQueryWithoutSlug,
};

export const commercePageContentPublicationId =
'dc9c0c95-9485-45c2-a76c-6923bb39b544';

export const commercePagesHash = generateContentHash(commercePageQuery);
export const commercePagesHash = generateContentHash({
contentTypeCode: commercePageQuery.contentTypeCode,
codes: commercePageQuery.slug,
});

export const mockCommercePages = {
number: 1,
Expand Down
Loading
Loading