da2k / curso-reactjs-ninja

915 stars 322 forks source link

Ajuda com o firebase firestore e autenticação #588

Closed pgstudies22 closed 2 years ago

pgstudies22 commented 2 years ago

Olá @fdaciuk! Tudo bem?

Decidi fazer um pequeno projeto com o que vi do firebase no curso, mas esbarrei em um problema que não estou conseguindo resolver.

O problema é o seguinte: ao fazer login e logout 2x consecutivas, na 2ª vez em que o logout é feito, a função de logout é executada 2x.

Se login e logout foram feitos pela 3ª vez, a função de logout é executada 3x, e assim por diante.

Vou deixar abaixo a parte do código que acredito que esteja relacionada ao problema (onAuthStateChanged e onSnapshot):

const logout = async unsubscribe => {
  try {
    await signOut(auth)
    console.log('usuário foi deslogado')

    if (unsubscribe) {
      console.log('unsubscribe:', unsubscribe)
      unsubscribe()
    }
  } catch (error) {
    console.log('error:', error)
  }
}

const handleAuthStateChanged = user => {
  // ...

  if (!user) {
    // ...
    buttonGoogle.addEventListener('click', login)
    linkLogout.removeEventListener('click', logout)
    return
  }

  const unsubscribe = onSnapshot(collectionPhrases, renderPhrases, error => console.log('Erro:', error))
  linkLogout.addEventListener('click', () => logout(unsubscribe))
  // ...
}

onAuthStateChanged(auth, handleAuthStateChanged)

Aqui tem um repositório do projeto.

Também vou deixar abaixo um vídeo de 52 segundos mostrando como reproduzir o problema.

Conseguiria me ajudar?

Um abraço.

https://user-images.githubusercontent.com/107188587/172850502-3a8c23b9-d683-40ce-81d8-c43644b141d1.mp4

fdaciuk commented 2 years ago

Oi @pgstudies22! O seu problema acontece por causa do código dessa linha.

Ali você passa como listener de evento de clique para o link de logou uma função inline. E nessa linha aqui, que é onde deveria acontecer a limpeza do listener de evento, ao invés de passar a mesma referência de função, você está passando uma outra função completamente diferente, aí o JS não tem como saber que as duas funções do add e do remove estão relacionadas.

Como a função unsubscribe está sendo criada sempre dentro do evento de handleAuthStateChanged, não teria uma forma simples de resolver o problema usando addEventListener e removeEventListener.

Nesse caso, eu recomendaria você usar o .onclick mesmo, vai ser mais simples de gerenciar, já que, usando o .onclick, somente um evento vai ser atrelado nesse link.

O "contra" dessa abordagem é que, se você quiser atrelar outro evento nesse mesmo link, ele pode ser sobrescrito pelo .onclick.

De maneira geral isso não é um problema, pois normalmente nós só atrelamos um evento por elemento mesmo.

Então a solução seria:

:)

pgstudies22 commented 2 years ago

aí o JS não tem como saber que as duas funções do add e do remove estão relacionadas.

Nossa, não usei a mesma referência de função em memória, é verdade!

Caraca, obrigado professor. Mesmo acompanhando o curso, quis fazer em js puro para treinar e não me atentei à esse detalhe.

Só pra ver se entendi... com .onclick, como ele usa o sinal de atribuição, toda vez que handleAuthStateChanged for executada, se a função que onclick recebe estava armazenada na memória (devido à alguma execução anterior), ela é substituída pela função criada na execução atual da handleAuthStateChanged (devido ao =).

Seria isso?


Além disso, para limpar da memória a função do .onclick quando o link de logout não está sendo exibido na tela, posso atribuir null para onclick dentro de if (!user), né?

const handleAuthStateChanged = user => {
  if (!user) {
    // ...
    linkLogout.onclick = null
    return
  }

  // ...
  linkLogout.onclick = () => logout(unsubscribe)
}

@fdaciuk

fdaciuk commented 2 years ago

Seria isso?

exatamente isso!

Além disso, para limpar da memória a função do .onclick quando o link de logout não está sendo exibido na tela, posso atribuir null para onclick dentro de if (!user), né?

Pode, mas não precisa. Quando o elemento não é mais usado, a memória que ele estava usando fica "solta", e o garbage collector faz a limpeza dessa área da memória, então não precisa se preocupar em limpar nesse caso :)

pgstudies22 commented 2 years ago

Pode, mas não precisa. Quando o elemento não é mais usado, a memória que ele estava usando fica "solta", e o garbage collector faz a limpeza dessa área da memória, então não precisa se preocupar em limpar nesse caso :)

Nos testes que fiz, depois de logar e deslogar, se onclick não receber null dentro de if (!user), o click no linkLogout (que não está visível no momento) executa a função que foi atribuída abaixo do if à onclick.

Acho que o evento permanece em memória pq linkLogout é usada em 2 escopos: handleAuthStateChanged e bloco if (!user). Não? 🤔

const handleAuthStateChanged = user => {
  // ...
  const linkLogout = document.querySelector('[data-js="logout"]')

  if (!user) {
    // ...
    linkLogout.onclick = null // se essa linha é removida, o callback é executado ao simular clique em linkLogout após deslogar
    return
  }

  // ...
  linkLogout.onclick = () => logout(unsubscribe)
}

Em nenhum momento linkLogout está sendo removido do DOM. Apenas a visibilidade dele está sendo manipulada com o CSS, saca?

@fdaciuk

fdaciuk commented 2 years ago

Ah, saquei! Se o elemento não sai do DOM, o evento continua válido. Então pode anular o evento manualmente, sem problemas :)

pgstudies22 commented 2 years ago

Legal!

Estou pensando em formas de refatorar, principalmente a função handleAuthStateChanged, que contém muita lógica.

Pensei em fazer algo como nesse pseudocódigo:

const handleAuthStateChanged = user => {
  mostraLinksDoNav(user)

  // ...

  const elements = {
    // objeto que contém elementos do DOM
  }

  if (!user) {
    lidaComUsuarioNaoLogado(elements)
    return
  }

  lidaComUsuarioLogado({ ...elements, user })
}

Acha que pode fazer sentido ir por este caminho?

@fdaciuk

fdaciuk commented 2 years ago

Pode ser uma boa. Tem que fazer pra ver como fica! Depois de pronto, vc bate o olho e vê se tem algo que poderia ficar melhor :)

Refactoring é um processo evolutivo: se o primeiro não ficou bom, você continua até achar que ficou como queria :)

A única coisa que não precisa é o destructuring ali do elements na função lidaComUsuarioLogado. Pode enviar duas entradas separadas mesmo, de elements e user :)

lidaComUsuarioLogado({ elements, user })
pgstudies22 commented 2 years ago

A única coisa que não precisa é o destructuring ali do elements na função lidaComUsuarioLogado. Pode enviar duas entradas separadas mesmo, de elements e user :)

Pq vc acha que o uso do spread nesse caso não faz sentido?

@fdaciuk

fdaciuk commented 2 years ago

Pq provavelmente você queria um objeto com duas chaves: elements e user, não? Com o spread vc vai ter uma chave user e as props de elements como chaves :)

pgstudies22 commented 2 years ago

Na verdade, eu estava pensando mais na simplicidade da declaração de parâmetro(s):

// com spread
const lidaComUsuarioLogado = ({ phrasesList, formAddPhrase, user }) => {
  // ...
}

// sem spread
const lidaComUsuarioLogado = ({ phrasesList, formAddPhrase }, user) => {
  // ...
}

Sem o spread, é necessário especificar destructuring + objeto... Era só por isso 😁

@fdaciuk

fdaciuk commented 2 years ago

Entendi.. mas aí tu não precisa de um objeto com elements, pode passar direto sem fazer o destructuring :D

pgstudies22 commented 2 years ago

Dessa forma?

lidaComUsuarioLogado(elements, user)

Pode ser tb. A desvantagem é que a função vai receber 2 argumentos, que é o que eu estava evitando =/

@fdaciuk

fdaciuk commented 2 years ago

Não, quis dizer pra vc fazer assim:

lidaComUsuarioLogado({ phrasesList, formAddPhrase, user })

Agora que eu percebi que estava falando da feature errada: era pra dizer que vc não precisava do do spread operator, não do destructuring :D

pgstudies22 commented 2 years ago

Ah, saquei hahah.

No exemplo anterior que dei, reduzi a quantidade de propriedades, apenas para o exemplo ficar mais enxuto xD


Neste último exemplo que vc deu, é como se phrasesList e formAddPhrase fossem consts. E não são.

Ao invés de declarar consts, eu preferi "agrupar" os elementos em um objeto, pq é mais fácil passar um objeto como argumento das funções:

const lidaComMudancaDeEstadoDeAuth = usuario => {
  // ...

  const elementos = {
    botaoGoogle: document.querySelector('[data-js="button-google"]'),
    formAdicionarFrase: document.querySelector('[data-js="add-phrase-form"]'),
    linkLogout: document.querySelector('[data-js="logout"]'),
    linkConta: document.querySelector('[data-js="account-link"]'),
    listaDeFrases: document.querySelector('[data-js="phrases-list"]')
  }

  if (!usuario) {
    lidaComUsuarioDeslogado(elementos)
    return
  }

  lidaComUsuarioLogado({ ...elementos, usuario })
}

Não é recomendado agrupar elementos do DOM em um objeto?

@fdaciuk

fdaciuk commented 2 years ago

É sim, pode fazer dessa forma se você precisa usar todos os elementos de uma vez. Mas se você só precisa de alguns, ainda que você tenha que escrever mais código para passar um a um, eu prefiro fazer dessa forma, pois fica mais claro o que a função vai, de fato, usar.

É mais verboso agora, mas vai ser muito mais fácil dar manutenção no futuro :)

pgstudies22 commented 2 years ago

Boa!

Muito obrigado pela ajuda, professor! Minhas dúvidas sobre o que discutimos aqui estão esclarecidas. Então, se quiser fechar a issue, fique à vontade =)

@fdaciuk

fdaciuk commented 2 years ago

Perfeito meu caro! Qualquer dúvida, fique à vontade para perguntar :)