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.

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

chore(customization_cart): customizations_replicated_products_in_cart #158

Merged
merged 6 commits into from
Mar 25, 2022

Conversation

oscargross
Copy link
Contributor

Issue-test:
Adicionar um produto com customização no cart, no momento em que voltamos ao produto e adicionamos um segundo produto com outra customização, o primeiro sofre alteração desses campos, ficando com as mesmas customizações do último adicionado ao cart.

Solution:
A prop customizations é adicionada aos items (product) do cart separadamente das demais props, assim, é alterada toda vez que o(s) campo(s) de customização(es) também é alterado. A PR faz com que os produtos que já estavam no carrinho sejam readicionados com suas props "antigas", sem edição, no momento em que um novo produto é adicionado.

Copy link
Member

@leomp12 leomp12 left a comment

Choose a reason for hiding this comment

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

Mas aí você foi na solução mais radical possível @oscargross 😬

Eu acho que o problema é só porque o objeto customizations é o mesmo entre os dois itens, por referência, já que a gente só duplica as props do item no primeiro nível em

const itemCopy = Object.assign({}, newItem)

nesse caso uma solução deve ser verificar se tem customizations, e nesse caso:

Object.assign(itemCopy.customizations, item.customizations)

em último caso você poderia copiar o objeto customizations deeply com lodash.clonedeep tipo acontece aí https://github.com/ecomplus/search-engine/blob/master/src/methods/reset.js

@oscargross
Copy link
Contributor Author

oscargross commented Mar 19, 2022

@leomp12 entendi
Só tem uma questão, as customizations dos produtos que já estão no cart são alteradas na edição desses campos, não quando o produto é adicionado. Na função setCustomizationOption aqui https://github.com/ecomplus/storefront/blob/master/%40ecomplus/storefront-components/src/js/TheProduct.js

Pode editar as customizations do segundo produto com o cart aberto para ver que já vai alterando o que já está no cart, assim, fazer deeply quando o novo for adicionado não vai adiantar 😬

Por isso pensei em pegar no localStorage , pois nos testes lá só é alterado quando a function addProdcut for chamada.

@leomp12
Copy link
Member

leomp12 commented Mar 20, 2022

Pode editar as customizations do segundo produto com o cart aberto para ver que já vai alterando o que já está no cart, assim, fazer deeply quando o novo for adicionado não vai adiantar

Mas você chegou a testar?
O negócio é descobrir porque o customizations de outro item é editado, eu acho que é porque eles compartilham o mesmo objeto por referência, o que seria resolvido com a sugestão que eu deixei, sacou?

@oscargross
Copy link
Contributor Author

oscargross commented Mar 21, 2022

@leomp12 monstro, tem razão, entendi nos testes a clonagem do estado anterior do cart.
Foi ok.. dá uma olhada se fica show assim pfv

Se passar, vi q tem essa issue sobre isso ecomplus/storefront#556

Copy link
Member

@leomp12 leomp12 left a comment

Choose a reason for hiding this comment

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

Ué, mas agora estamos mantendo o objeto do carrinho duplicado em cada instância ? Acho que não gasta isso não sô 😄

em último caso você poderia copiar o objeto customizations deeply com lodash.clonedeep tipo acontece aí https://github.com/ecomplus/search-engine/blob/master/src/methods/reset.js

O clonedeep seria do customizations apenas, dentro do método addItem mesmo:

if (itemCopy.customizations) {
  itemCopy.customizations = cloneDeep(item.customizations)
}

isso se um assign ainda mais simples não for suficiente:

if (itemCopy.customizations) {
  Object.assign(itemCopy.customizations, item.customizations)
}

@leomp12
Copy link
Member

leomp12 commented Mar 22, 2022

O problema é só com customizations então não tem porque clonar todo o objeto do carrinho, todos os itens e etc, faz sentido? Nas duas implementações você fez isso na verdade 😛

O fato de JSON.parse resolver o problema (entendi que você testou isso e deu certo...) sugere que o issue é só com objetos que deveriam ser diferentes e estão mantidos os mesmos por referência, com o parse você cria um espaço novo em memória para um objeto novo, mas o ponto é que você não precisa fazer isso com o carrinho inteiro sempre que um item é adicionado, não há motivo pra isso (especialmente manipulando JSON e storage sempre), se o problema é só o objeto customizations basta que na adição do item ao carrinho você clone esse objeto no item atual apenas, justamente para "criar outra a referência", já é suficiente para que todos os itens terão customizations com objetos diferentes sempre, faz sentido?

Object.assign já faz isso também, só que só no "primeiro nível", como o customizations é um array de nested objects pode ser que o problema persista, nesse caso o clonedeep deve resolver.

Copy link
Member

@leomp12 leomp12 left a comment

Choose a reason for hiding this comment

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

@oscargross testei aqui e só isso:

if (newItem.customizations) {
  newItem.customizations.forEach((customization, i) => {
    itemCopy.customizations[i] = Object.assign({}, customization)
  })
}

já é suficiente, evita o lodash.clonedeep (portanto deixa de adicionar uma dependência) e principalmente não copia o cart todo...

Comitei no seu branch pra aprovar a PR.

O fato de JSON.parse resolver o problema (entendi que você testou isso e deu certo...) sugere que o issue é só com objetos que deveriam ser diferentes e estão mantidos os mesmos por referência

Valeu por identificar isso pra nós 💜

@leomp12 leomp12 merged commit 51fd0f2 into ecomplus:master Mar 25, 2022
@oscargross
Copy link
Contributor Author

oscargross commented Mar 25, 2022

@leomp12 Opa meu querido, enviei agr um commit só clonando o customizations do ultimo product como sugeriu, não todo o cart como estava.. mas de qualquer forma seu commit tbm soluciona.

Abração!! 💚

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants