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

wants to merge 1 commit into from

Conversation

richard190m
Copy link
Contributor

Description

Fix commerce pages on server side by:

  • Fix serverInitialState to include strategy prop
  • Fix hooks useCommercePages and useContentPages to use slug as hash to redux store

Dependencies

Checklist

  • The commit message follows our guidelines
  • Tests for the respective changes have been added
  • The code is commented, particularly in hard-to-understand areas
  • The labels and/or milestones were added

Disclaimer

By sending us your contributions, you are agreeing that your contribution is made subject to the terms of our Contributor Ownership Statement

@richard190m richard190m added 📦 react Relative to the `@farfetch/blackout-react` package 📦 redux Relative to the `@farfetch/blackout-redux` package labels Jan 9, 2024
@github-actions github-actions bot added the type: bug Something isn't working properly label Jan 9, 2024
@richard190m richard190m force-pushed the fix/serverInitialState-commerce-pages branch from 464f0ce to ca1a08e Compare January 9, 2024 14:52
@github-actions github-actions bot added type: bug Something isn't working properly and removed type: bug Something isn't working properly labels Jan 9, 2024
@richard190m richard190m changed the title fix: commerce pages in ssr fix(react|redux): commerce pages in ssr Jan 9, 2024
@richard190m richard190m force-pushed the fix/serverInitialState-commerce-pages branch from ca1a08e to 47eab25 Compare January 10, 2024 15:25
@github-actions github-actions bot added type: bug Something isn't working properly and removed type: bug Something isn't working properly labels Jan 10, 2024
@richard190m richard190m force-pushed the fix/serverInitialState-commerce-pages branch from 47eab25 to 14a1d68 Compare January 10, 2024 15:27
@github-actions github-actions bot added type: bug Something isn't working properly and removed type: bug Something isn't working properly labels Jan 10, 2024
@richard190m richard190m marked this pull request as ready for review January 10, 2024 15:28
@richard190m richard190m changed the title fix(react|redux): commerce pages in ssr fix(react|redux): commerce pages in ssr not working Jan 10, 2024
@richard190m richard190m force-pushed the fix/serverInitialState-commerce-pages branch from 14a1d68 to 2429d79 Compare January 10, 2024 15:35
@github-actions github-actions bot added type: bug Something isn't working properly and removed type: bug Something isn't working properly labels Jan 10, 2024
@richard190m richard190m force-pushed the fix/serverInitialState-commerce-pages branch from 2429d79 to 8f7947e Compare January 10, 2024 16:02
@github-actions github-actions bot added type: bug Something isn't working properly and removed type: bug Something isn't working properly labels Jan 10, 2024
@richard190m richard190m force-pushed the fix/serverInitialState-commerce-pages branch from 8f7947e to d0b2abc Compare January 10, 2024 16:22
@github-actions github-actions bot removed the type: bug Something isn't working properly label Jan 10, 2024
@github-actions github-actions bot added the type: bug Something isn't working properly label Jan 10, 2024
boliveira
boliveira reviewed Jan 15, 2024
View reviewed changes
packages/redux/src/contents/actions/factories/fetchCommercePagesFactory.ts
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

boliveira
boliveira reviewed Jan 15, 2024
View reviewed changes
packages/redux/src/contents/actions/factories/fetchCommercePagesFactory.ts
@@ -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.

boliveira
boliveira reviewed Jan 15, 2024
View reviewed changes
packages/react/src/contents/hooks/useContentPage.ts

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

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.

boliveira
boliveira reviewed Jan 15, 2024
View reviewed changes
packages/react/src/contents/hooks/types/useCommercePages.types.ts
@@ -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

boliveira
boliveira reviewed Jan 15, 2024
View reviewed changes
packages/redux/src/contents/serverInitialState.ts
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.

@dnamorim dnamorim added this to the dev-main-23-01-24 milestone Jan 26, 2024
@dnamorim dnamorim changed the base branch from main to dev-main-23-01-24 January 26, 2024 14:27
@dnamorim dnamorim force-pushed the dev-main-23-01-24 branch 3 times, most recently from 1729a08 to b9d6190 Compare January 29, 2024 11:29
Base automatically changed from dev-main-23-01-24 to main January 30, 2024 17:39
@boliveira boliveira changed the base branch from main to dev-main-06-02-2024 January 30, 2024 17:42
Base automatically changed from dev-main-06-02-2024 to main February 6, 2024 16:59
@dnamorim dnamorim changed the base branch from main to dev-main-13-02-24 February 6, 2024 17:10
Base automatically changed from dev-main-13-02-24 to main February 14, 2024 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boliveira boliveira boliveira left review comments

@vinicaval vinicaval vinicaval approved these changes

@joaoprcosta joaoprcosta joaoprcosta approved these changes

@edupaulos edupaulos edupaulos approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
📦 react Relative to the `@farfetch/blackout-react` package 📦 redux Relative to the `@farfetch/blackout-redux` package type: bug Something isn't working properly
Milestone
dev-main-13-02-24
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@richard190m @boliveira @vinicaval @joaoprcosta @edupaulos @dnamorim