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

[oFix] Refactoring SellerWrapper and Adding Custom Hooks #139

Merged
merged 2 commits into from
Dec 19, 2023
Merged

[oFix] Refactoring SellerWrapper and Adding Custom Hooks #139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
41c9720
fix: fixing on seller wrapper when there is no seller available
arturmagalhaesjr Nov 30, 2023
09cd9e9
added: added a dependency on useEffect call and selectedSeller
arturmagalhaesjr Dec 11, 2023
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

## [Unreleased]

### Fixed

- Fixing on seller wrapper when there is no seller available
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought that this PR was just a refactoring, does it also fix a bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I found a bug and leverage the PR to refactor the code and the logic/algorithms


## [1.28.1] - 2023-11-06

### Fixed
Expand Down
91 changes: 1 addition & 90 deletions react/SellerWrapper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,92 +1,3 @@
import React, { useCallback, useEffect, useRef, useState } from 'react'
import { useProduct, useProductDispatch } from 'vtex.product-context'
import { useCssHandles } from 'vtex.css-handles'
import type { Item } from 'vtex.product-context/react/ProductTypes'

import useSeller from './hooks/useSeller'

const CSS_HANDLES = ['sellerWrapper', 'loadingSeller']
const SELECT_ITEM_EVENT = 'SET_SELECTED_ITEM'

type SellerWrapperProps = {
children: React.ReactNode
}

const SellerWrapper = ({ children }: SellerWrapperProps) => {
const { seller } = useSeller()
const dispatch = useProductDispatch()
const { selectedItem, product } = useProduct() ?? {}
const latestItem = useRef((null as unknown) as Item)
const handles = useCssHandles(CSS_HANDLES)
const [loadingSeller, setLoadingSeller] = useState(true)

const addSellerDefaultToItem = useCallback(
itemSeller => ({
...itemSeller,
sellerDefault: seller?.includes(itemSeller.sellerId),
}),
[seller]
)

useEffect(() => {
const shouldDispatchSelectItem =
!!seller && !!product && !!selectedItem && !!dispatch

if (!shouldDispatchSelectItem) return
const newCurrentSelectedItem =
product?.items?.find(item =>
item.sellers?.find(
itemSeller =>
seller?.includes(itemSeller.sellerId) &&
itemSeller.commertialOffer.AvailableQuantity > 0
)
) ?? ({} as Item)

if (!newCurrentSelectedItem) {
return
}

const { sellers } = newCurrentSelectedItem
const selectedItemWithSeller = {
...newCurrentSelectedItem,
sellers: sellers.map(addSellerDefaultToItem),
}

if (
JSON.stringify(latestItem.current) ===
JSON.stringify(selectedItemWithSeller)
) {
return
}

dispatch?.({
type: SELECT_ITEM_EVENT,
args: {
item: selectedItemWithSeller,
},
})

setLoadingSeller(false)

latestItem.current = selectedItemWithSeller
}, [
seller,
product,
selectedItem,
dispatch,
addSellerDefaultToItem,
latestItem,
])

return (
<div
className={`${handles.sellerWrapper} ${
loadingSeller ? handles.loadingSeller : ''
}`}
>
{children}
</div>
)
}
import SellerWrapper from './components/SellerWrapper/SellerWrapper'

export default SellerWrapper
30 changes: 30 additions & 0 deletions react/components/SellerWrapper/SellerWrapper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import React, { useEffect } from 'react'
import { useCssHandles } from 'vtex.css-handles'
import { useProduct } from 'vtex.product-context'

import { useSelectSeller } from '../../hooks/useSelectSeller'

type SellerWrapperProps = {
children: React.ReactNode
}

const CSS_HANDLES = ['sellerWrapper', 'sellerWrapperNoSeller']

const SellerWrapper = ({ children }: SellerWrapperProps) => {
const handles = useCssHandles(CSS_HANDLES)
const { currentSelectedItem, selectSeller } = useSelectSeller()

const { selectedItem } = useProduct() ?? {}

useEffect(() => {
selectSeller({ selectedItem })
}, [selectedItem, selectSeller])
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 weird, this selectedItem variable is a dep of this useEffect, but it's not directly being used. Is the usage indirect instead? And if so, can it be changed so that the usage is direct (for example, passing it to selectSeller)? That would make the code easier to read and harder to introduce regressions (such as someone just removing this dep in the future, believing it's not necessary).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct, I just added the dependency. However, in this logic, we need to force the seller whatever takes. So, that's the reason the selectedItem is assigned there.
It's a listener function rather than a side effect by itself.
Anyway, I've added the dependency to ensure the best practices are done.


const className = currentSelectedItem
? `${handles.sellerWrapper}`
: `${handles.sellerWrapper} ${handles.sellerWrapperNoSeller}`
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 different from the original code that was refactored, it used handles.loadingSeller instead and it depended on the loading state, not the selected state. Was this behavior changed on purpose or am I missing something?

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 part of refactoring, I noticed the loading seller handle has never been displayed, once the flow is synchronous and there is no "loading" time. It prevents the layout shifting.
Besides that, the [oFix] was the first store to use this functionality and we could quickly get back to them with the bug fix.
No regression bugs in this case.


return <div className={className}>{children}</div>
}

export default SellerWrapper
72 changes: 72 additions & 0 deletions react/hooks/useSelectSeller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
import { useProduct, useProductDispatch } from 'vtex.product-context'
import { useCallback, useMemo, useRef, useState } from 'react'
import type { Item } from 'vtex.product-context/react/ProductTypes'

import useSeller from './useSeller'

const SELECT_ITEM_EVENT = 'SET_SELECTED_ITEM'

export const useSelectSeller = () => {
const { seller } = useSeller()
const { product } = useProduct() ?? {}
const latestItem = useRef((null as unknown) as Item)
const [loading, setLoading] = useState(true)
const productDispatch = useProductDispatch()

const addSellerDefaultToItem = useCallback(
itemSeller => ({
...itemSeller,
sellerDefault: seller?.includes(itemSeller.sellerId),
}),
[seller]
)

const currentSelectedItem = useMemo(
() =>
product?.items?.find(item =>
item.sellers?.find(
itemSeller =>
seller?.includes(itemSeller.sellerId) &&
itemSeller.commertialOffer.AvailableQuantity > 0
)
) ?? null,

[seller, product]
)

const selectSeller = ({
selectedItem,
}: {
selectedItem: Item | null | undefined
}) => {
if (!currentSelectedItem || !selectedItem) {
return
}

// just to keep the dependency
setLoading(true)
const { sellers } = (currentSelectedItem as unknown) as Item
const selectedItemWithSeller = {
...((currentSelectedItem as unknown) as Item),
Comment on lines +48 to +50
Copy link
Contributor

Choose a reason for hiding this comment

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

These as unknown as Item castings didn't seem to be necessary before, do you know why they were needed now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was done before.
CleanShot 2023-12-11 at 15 26 10@2x

Once you create a useReff as null, to cast it properly, you must cast to unknown and then to Item in this case to avoid typing errors, otherwise, a TS error is going to occur.

sellers: sellers.map(addSellerDefaultToItem),
}

if (
JSON.stringify(latestItem.current) ===
JSON.stringify(selectedItemWithSeller)
) {
return
}

productDispatch?.({
type: SELECT_ITEM_EVENT,
args: {
item: selectedItemWithSeller,
},
})
setLoading(false)
latestItem.current = selectedItemWithSeller
}

return { loading, seller, currentSelectedItem, selectSeller }
}
0 react/hooks/useSeller.tsx → react/hooks/useSeller.ts
View file
Open in desktop
File renamed without changes.
Loading