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

feat(use-user): cache user state to localStorage #443

Merged
merged 2 commits into from
Jun 9, 2022

Conversation

filipedeschamps
Copy link
Owner

Este PR faz par com o PR #440 do @aprendendofelipe e a tentativa aqui foi fazer a mesma coisa proposta por ele, mas usando o swr. Não trouxe para cá todas as implementações do PR dele, queria apenas rabiscar algo para ele ver.

Então as características são:

  1. Componente começa com o loginStatus como "loading" e que pode resolver para "logged-in" ou "logged-out".
  2. Com isso, na interface posso escolher não renderizar nenhum item no menu enquanto não resolver para algum dos dois status acima.
  3. Se resolver para "logged-out" aparece o menu de Login / Cadastrar.
  4. Se resolver para "logged-in" aparece o dropdown menu com o username.
  5. Decidir estar logado ou não é resolvido através da existência ou não do item user no localStorage.
  6. Se existir esse item, ele é injetado como fallbackData no swr para que independente da revalidação que ele esteja fazendo contra o endpoint real, o user será retornado instantaneamente.
  7. Se o swr conseguir revalidar, o user é atualizado.
  8. Se não conseguir revalidar pelo fato do endpoint /user retornar 401 ou 403, o item user é deletado do localStorage e o loginStatus é setado como "logged-out".
  9. A variável shouldRevalidate controla se o swr deve fazer uma tentativa de bater no endpoint /user e essa variável é apenas true se loginStatus estiver setado como "logged-in". Se estiver como false, o swr não tenta bater na rota.
  10. Na atual implementação, usuários precisarão se logar novamente, mas a ideia seria fazer um deploy intermediário que vai populando por um tempo o localStorage do pessoal, até que decidirmos virar a chave.
  11. Url de teste: https://tabnews-ffuzi3g7w-tabnews.vercel.app/

@vercel
Copy link

vercel bot commented Jun 7, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
tabnews ✅ Ready (Inspect) Visit Preview Jun 8, 2022 at 2:41AM (UTC)

Copy link
Collaborator

@aprendendofelipe aprendendofelipe left a comment

Choose a reason for hiding this comment

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

Conditional Fetching... 🤯 Mas que implementação delicinha! 😍 Muito mais elegante que a minha!
E, se eu não perdi nada, a única coisa que a minha faz a mais é não pedir um novo login para os usuários com sessão ativa que ainda não tenham o user salvo no storage.
Proposital, imagino, pois seria só um detalhezinho a mais para implementar. Se não foi proposital, eu posso sugerir algo... 🚀

@filipedeschamps
Copy link
Owner Author

Conditional Fetching... 🤯 Mas que implementação delicinha! 😍 Muito mais elegante que a minha!

Show!!! Gosto de usar o swr nessa situação, pois ele faz o deduping automático das requests (dedupingInterval = 2000), então o useUser() pode ser chamado em seguida várias vezes que o swr vai bater na rota só uma vez e compartilhar o resultado com todo mundo.

E, se eu não perdi nada, a única coisa que a minha faz a mais é não pedir um novo login para os usuários com sessão ativa que ainda não tenham o user salvo no storage.
Proposital, imagino, pois seria só um detalhezinho a mais para implementar. Se não foi proposital, eu posso sugerir algo... 🚀

Total!!! Inclusive, fiz um push agora que toma nota do Stage 1 e Stage 2 que deveríamos lançar essa feature. No Stage 1 o hook vai continuar batendo no /user independente de qualquer coisa e isso vai servir para popular o localStorage da galera. A gente pode deixar por alguns bons dias, talvez mais do que uma semana, e dai mudar para o Stage 2 e encerrar a issue.

E sobre o /login, achei muito charmoso da forma que você fez, o redirect ficou perfeito. Topa fazer takeover dessa branch e lapidar essa parte? Eu não gostei do jeito que eu fiz, seria legal poder usar o hook ali também para guardar os dados no localStorage... eu acabei fazendo um fetch doido ali no meio do nada 🤝


Em paralelo o teste falhou, mas foi no npm ci, what? 😛 Rodei de novo e foi 👍

image

@aprendendofelipe
Copy link
Collaborator

Topa fazer takeover dessa branch e lapidar essa parte?

Opa, amanhã devo conseguir arrumar um tempo para ver isso... Talvez consiga fazer algo parecido com o mutate do swr 🤝

@filipedeschamps
Copy link
Owner Author

Show! Mas acho que o mutate só seria válido se a gente não estivesse dando um full refresh na página ao navegar.

@filipedeschamps
Copy link
Owner Author

@aprendendofelipe eu tava pensando aqui: e se a gente já fizer deploy desse PR para já ir populando o localStorage da turma e daí ir iterando as lapidações em outros PRs?

Então fora o que a gente conversou do /login, tem uma outra lapidação que da pra fazer que é o componente do Content usar loginStatus ao invés do isLoading, porque o loginStatus é instantâneo e o isLoading precisa aguardar a resposta do /user 🤝

@aprendendofelipe
Copy link
Collaborator

@aprendendofelipe eu tava pensando aqui: e se a gente já fizer deploy desse PR para já ir populando o localStorage da turma e daí ir iterando as lapidações em outros PRs?

Oi @filipedeschamps, ainda não tive tempo de mexer com isso, mas ontem, esperando o sono vir, me surgiu na cabeça algumas questões que precisaria testar para confirmar. Acho que na implementação desse PR #443 está sendo lido o localStorage para cada componente que utiliza do useUser (só o fetch duplicado é evitado).

Por exemplo, uma página de um conteúdo com 20 respostas irá ler a mesma informação do localStorage mais de 20 vezes ao invés de usar a informação em memória.

No PR #440 o retorno é memoizado e o estado é único para todos os componentes que utilizam o useUser por causa do Context.Provider.

Se não se importar com as diversas leituras do localStorage, pra mandar para produção só precisa corrigir um bug no Content - verificar user? e não só username no handleSubmit - Só falta a interrogação no user do trecho abaixo:

const handleSubmit = useCallback(
      async (event) => {
        event.preventDefault();
        if (!user.username) {
          router.push('/login');
          return;

Então fora o que a gente conversou do /login, tem uma outra lapidação que da pra fazer que é o componente do Content usar loginStatus ao invés do isLoading, porque o loginStatus é instantâneo e o isLoading precisa aguardar a resposta do /user 🤝

Acho o isLoading do Content está em locais que faz sentido aguardar a revalidação antes de permitir, por exemplo, publicar. Talvez mudaria só no condicional do localStorageContent

@aprendendofelipe
Copy link
Collaborator

Show! Mas acho que o mutate só seria válido se a gente não estivesse dando um full refresh na página ao navegar.

Seria ainda mais útil nesse caso, mas também dá pra usar ele pra forçar um novo fetch

@aprendendofelipe
Copy link
Collaborator

Outra coisa que pensei aqui, e que daria problema nas duas implementações diferentes (na sua, só na fase 2), é que alguma configuração de privacidade que impeça a gravação em localStorage vai tornar impossível manter o status de usuário logado.

@filipedeschamps
Copy link
Owner Author

Por exemplo, uma página de um conteúdo com 20 respostas irá ler a mesma informação do localStorage mais de 20 vezes ao invés de usar a informação em memória.

No PR #440 o retorno é memoizado e o estado é único para todos os componentes que utilizam o useUser por causa do Context.Provider.

Ahh interessantíssimo! Você acha que é possível então misturar as duas estratégias dos dois PRs?

Acho o isLoading do Content está em locais que faz sentido aguardar a revalidação antes de permitir, por exemplo, publicar. Talvez mudaria só no condicional do localStorageContent

Mas aí que tá, nós temos duas garantias:

  1. A primeira que na verdade não é uma garantia, mas que especula que o usuário já está logado, por conta do user no localStorage (e com a revalidação acontecendo no background batendo no /user).
  2. A garantia do backend (batendo na rota /contents e passando pela validação real da sessão), pois se a pessoa conseguir furar a primeira garantia (por algum evento raro e de não ter tempo do /user revalidar no background), ela cai na segunda garantia. Faz sentido?

Seria ainda mais útil nesse caso, mas também dá pra usar ele pra forçar um novo fetch

Ahhh que delicinha!!! Perfeito 👍

Outra coisa que pensei aqui, e que daria problema nas duas implementações diferentes (na sua, só na fase 2), é que alguma configuração de privacidade que impeça a gravação em localStorage vai tornar impossível manter o status de usuário logado.

De fato, mas prefiro ver dar problema para ser sincero, pois nesse tipo de preocupação pode-se envolver também os cookies (ou até JavaScript).


Então faço a sugestão de novo de você fazer takeover nessa branch e decidir quando achar que está pronta para deploy (ou se devemos abandoná-la) 🤝 com isso, consigo paralelizar e trabalhar na issue de auditoria da milestone 4 👍

@aprendendofelipe
Copy link
Collaborator

@filipedeschamps deixa comigo! 🤝

  1. Pela sua estratégia de fase1 e fase2, agora ficou claro para mim que você prefere, por enquanto, continuar batendo no endpoint mesmo sem cache enquanto não popula o localStorage, ao contrário do que eu tinha feito que mandava o usuário para a tela de login e redirecionava se já houvesse sessão. Então dá pra implementar logo um código só para ir populando, que pode ser testado rápido e já ir para produção. Em seguida dá pra fazer com mais calma o restante.

  2. Segundo passo, usar logo o cache para renderizar os componentes que precisam do user. Dá sim, com certeza, para juntar o melhor das duas estratégias! Mas preciso saber, pretende manter o swr por algum outro motivo ou era só pela deduplicação das chamadas? Pergunto, pois, caso eu não encontre uma maneira de evitar o excesso de leituras do localStorage com o swr, parto para algo mais parecido com o do PR #440 que também evitava chamadas repetidas. De qualquer maneira, vou primeiro insistir em manter o swr para me aprofundar mais.

  3. Content usando o loginStatus 👍

  4. Por fim, retirar as chamadas desnecessárias ao endpoint. E sobre a questão de privacidade impedindo o uso do localStorage, acho que existe um erro específico que pode ser tratado para forçar bater no endpoint.

@filipedeschamps
Copy link
Owner Author

  1. Show, isso 🤝
  2. Basicamente era isso e eu acho que o código fica mais simples com o swr, mas sem apego, ele não é necessário. Em paralelo, ele tem o sistema de cache dele também, tem exemplo na documentação usando o localStorage.
  3. 🤝
  4. Show! Mas eu realmente não implementaria nada a respeito disso agora.

@vercel
Copy link

vercel bot commented Jun 8, 2022

@aprendendofelipe is attempting to deploy a commit to the TabNews Team on Vercel.

To accomplish this, @aprendendofelipe needs to request access to the Team.

Afterwards, an owner of the Team is required to accept their membership request.

If you're already a member of the respective Vercel Team, make sure that your Personal Vercel Account is connected to your GitHub account.

@aprendendofelipe
Copy link
Collaborator

Parte 1 - ok 😉

@filipedeschamps
Copy link
Owner Author

Massa! Isso significa que podemos fazer o deploy, correto?

@aprendendofelipe
Copy link
Collaborator

Massa! Isso significa que podemos fazer o deploy, correto?

Fiz uma bateria de testes aqui para lhe responder e percebi que, quando o usuário não estava logado, o swr ficava insistindo no endpoint e retornando vários 403 e isso não acontecia antes.

O que acontece agora é que o fetcher está gerando um erro que é passado para o swr, e com isso ele fica tentando de novo.

Por hora setei o shouldRetryOnError = false nas opções do swr e parou de ficar martelando o endpoint, mas acho que isso vale uma discussão se o retorno com status 401/403 deveria mesmo gerar um erro para o swr, pois na verdade o fetch obteve sucesso.

Tirando o erro maluco das dependências, pode mandar pra deploy 🚀

@filipedeschamps filipedeschamps changed the base branch from main to cache-use-user2 June 9, 2022 16:24
@filipedeschamps
Copy link
Owner Author

Por hora setei o shouldRetryOnError = false nas opções do swr e parou de ficar martelando o endpoint, mas acho que isso vale uma discussão se o retorno com status 401/403 deveria mesmo gerar um erro para o swr, pois na verdade o fetch obteve sucesso.

Interessantíssimo, de fato ele vai fazer isso por padrão. Na primeira implementação que fiz, eu não dava throw e deletava o localStorage lá no fetcher mesmo, mas daí eu não tinha como avisar o componente que a pessoa deslogou para colocar ele no modo "logged-out" e atualizar a interface.

E não vejo problema com a flag que você colocou. Em situações assim eu gosto do caminho feliz ser de uma forma, e o caminho não feliz ser o caminho do throw. Pelo menos o backend TabNews inteiro foi arquitetado assim (vamo ve se vai se sustentar 😂 ).

Bom, estou fazendo merge numa branch intermediária pra forçar o deploy em homologação 👍

@filipedeschamps filipedeschamps merged commit df5663f into cache-use-user2 Jun 9, 2022
@filipedeschamps filipedeschamps deleted the cache-use-user branch June 9, 2022 16:41
@aprendendofelipe
Copy link
Collaborator

É @filipedeschamps , pesquisei bastante e até analisei o código do swr por causa das funcionalidades beta que ainda não estão documentadas pra ver se encontrava uma brecha pra usar ele da maneira que precisamos e sem ficar lendo repetidamente o localStorage, mas não encontrei nenhum jeito. Então acho que amanhã eu vou de Context.Provider mesmo para fazer as próximas etapas 👍

@filipedeschamps
Copy link
Owner Author

Show meu caro, obrigado por ficar em cima disso 🤝

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.

None yet

2 participants