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): use localStorage cached user #440

Merged
merged 6 commits into from
Jun 14, 2022
Merged

Conversation

aprendendofelipe
Copy link
Collaborator

Usando localStorage no lugar de cookie, atende ao #336 useUser(): somente bater no endpoint se existir cookie indicando sessão

Fornece um UserProvider, onde o useUser somente verifica a sessão do usuário se encontrar user no localStorage. Diminuindo as chamadas para a API quando é certo que o usuário não está autenticado.

Provisoriamente (ou não) foi incluída a verificação da sessão na tela de login, mesmo sem encontrar user no localStorage. Assim os usuários que ainda não acessaram a versão nova não precisarão realizar novo login.

@vercel
Copy link

vercel bot commented Jun 7, 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 Author

@filipedeschamps, está dando erro nos testes, mas "na minha máquina funciona". hahahaha
Falando sério, nesse PR não foi feita nenhuma alteração em nada relacionado ao teste que está dando erro.

@filipedeschamps
Copy link
Owner

image

Que interessantíssimo né? É na nova função recursiveInjectChildrenDeepCount() no /children, mas eu também aqui localmente não consigo reproduzir. Eu fiz um git fetch && git reset --hard origin/main pra forçar estar na main e passa tudo. E de fato, seu PR não deveria ter nenhuma influência nas rotas da API.

De qualquer forma, em paralelo eu vou analisar com mais calma o seu PR e também queria testar a alternativa de só usar o cache local do swr 🤝

@filipedeschamps
Copy link
Owner

Tem algo muito estranho acontecendo mesmo, eu vi que você tinha re-rodado os testes no CI e não tinham passados, mas eu forcei rodar de novo e passou. Só que essa branch local, não passou 😂

image

Estou investigando aqui 🤝

@filipedeschamps
Copy link
Owner

Olha o que eu encontrei no tests/integration/api/v1/contents/[username]/[slug]/children no teste From "root" content with "deleted" status 🤩

image

@aprendendofelipe
Copy link
Collaborator Author

queria testar a alternativa de só usar o cache local do swr

A não ser que a ideia que dá título à Issue tenha mudado radicalmente, acho que não funcionará com o swr, pois não deve ser possível fazer ele revalidar somente se tiver cache de sessão. Acho que só dá pra fazer o contrário, que é só fazer o fetch se não houver cache.

O UserProvider desse PR fornece o user de localStorage e assume como válido enquanto não faz o fetch para revalidar (enquanto isso matem o isLoading = true para qualquer caso necessário). Mas, para atender à #336, se não houver cache, ele nem tenta revalidar, pois é provável que o usuário não esteja logado.

Caso seja necessário ignorar o cache, basta chamar o fetchUser do useUser para forçar a revalidação o usuário. No momento isso só está ocorrendo na tela de login, mas pode ser chamado sempre que houver qualquer alteração no cadastro do usuário.

Dentro do useUser dá pra expor também o setUser quando isso fizer sentido, para, por exemplo, implementar uma interface otimista ao fazer edições do usuário.

Daria pra usar também o setUser para economizar uma nova chamada ao endpoint user após realizar login se o que retornasse do endpoint sessions contivesse todos os dados para montar o cache do user.


Olha o que eu encontrei no tests/integration/api/v1/contents/[username]/[slug]/children no teste From "root" content with "deleted" status 🤩

Alguém apressado não estava afim de aguardar... hahaha

@filipedeschamps
Copy link
Owner

@aprendendofelipe com o PR #442 se você fizer o rebase esse tipo de problema não deve mais acontecer 🤝

@aprendendofelipe
Copy link
Collaborator Author

@aprendendofelipe com o PR #442 se você fizer o rebase esse tipo de problema não deve mais acontecer 🤝

Done! 🤝

pages/_app.public.js Outdated Show resolved Hide resolved
@aprendendofelipe aprendendofelipe requested a review from tembra June 7, 2022 17:35
@aprendendofelipe aprendendofelipe added the front Envolve modificações no frontend label Jun 7, 2022
@aprendendofelipe aprendendofelipe changed the title perf(useUser): fewer 403 returns in API [WIP] feat(use-user): use localStorage cached user Jun 11, 2022
@aprendendofelipe aprendendofelipe changed the title [WIP] feat(use-user): use localStorage cached user feat(use-user): use localStorage cached user Jun 11, 2022
@aprendendofelipe
Copy link
Collaborator Author

aprendendofelipe commented Jun 11, 2022

@filipedeschamps, está concluída feature para usar o user do localStorage antes de revalidar. 🚀

Já foram realizadas as mudanças relacionadas que conversamos aqui, nas Issues #336 e #446, e no PR #443. E a fase 2 também já está no código do useUser, só que comentada. Está pronta para entrar em ação quando eliminar os acessos ao endpoint quando não tiver cache do usuário.

Fora tudo que conversamos antes, precisei dar mais uma refatorada no Content e mudar detalhes relacionados ao husky, coisas que explico a seguir.

O Content já tinha alguns problemas em determinadas situações, pois ele reinicializava completamente no mínimo duas vezes. Com o cache de usuários acrescentamos uma reinicialização (static/stale/revalidated) e ficou visível o problema de performance. Por exemplo, dava pra ver o texto piscando na tela de publicar quando existia rascunho salvo. Isso deixou claro que o componente carregava do zero a cada mudança de estado. Para corrigir, eu separei os componentes relacionados a cada modo (view, edit, compact e deleted), mas, para não ser radical demais, mantive tudo no mesmo arquivo e na mesma ordem que já estava antes.

Agora sobre o huscky, que maravilha! Acho que vai evitar muitos problemas. Só que sem essa pequena alteração que fiz (commit-msg, lint:prettier e lint:fix), eu não conseguia de jeito nenhum fazer um commit sequer. Antes eu já deixava o package.json do jeito que está nesse PR para conseguir rodar o lint no Windows, mas eu não commitava essa alteração. Só que com o husky, o que não está staged ele retira (temporariamente) a alteração do arquivo na hora de rodar as verificações. Deve fazer isso para testar o que realmente vai ser commitado, então, sem commitar essa alteração, não tinha como fazer qualquer outro commit no Windows.

Bom, é isso! Caso queira fazer squash pelo GitHub, pode mandar ver. Eu tentei fazer pelo terminal, mas não está dando certo. Acho que não é o huscky se intrometendo, mas não investiguei.

@filipedeschamps
Copy link
Owner

Showwwwwwww @aprendendofelipe quero ver com calma agora no início da semana, sensacional 😍

@aprendendofelipe
Copy link
Collaborator Author

@filipedeschamps testa essa versão... No Windows funcionou
"lint:prettier": "prettier --check \u0022**/*.{json,js,jsx,ts,tsx}\u0022",

Já essa outra não rolou no Windows
"lint:prettier": "prettier --check \u0027**/*.{json,js,jsx,ts,tsx}\u0027",

@aprendendofelipe
Copy link
Collaborator Author

@filipedeschamps testa essa versão... No Windows funcionou

O prettier funcionou mesmo, mas ao rodar o lint:fix, o \u0027 é automaticamente substituído por \" que é a versão que dá erro no GitHub Actions

@filipedeschamps
Copy link
Owner

filipedeschamps commented Jun 14, 2022

@aprendendofelipe veja se aí roda assim: "lint:prettier": "prettier --check ."

@aprendendofelipe
Copy link
Collaborator Author

@aprendendofelipe veja se aí roda assim: "lint:prettier": "prettier --check ."

Tava testando isso agora mesmo... kkkkkkk... Sim... Mas precisa colocar algumas coisas no .prettierignore

@vercel
Copy link

vercel bot commented Jun 14, 2022

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

Name Status Preview Updated
tabnews ✅ Ready (Inspect) Visit Preview Jun 14, 2022 at 1:18AM (UTC)

@filipedeschamps
Copy link
Owner

@aprendendofelipe fiz um force push, veja se não fiz nenhuma besteira no PR (pois deu conflito no rebase) 🤝

@aprendendofelipe
Copy link
Collaborator Author

Aeeeee

@filipedeschamps
Copy link
Owner

E a branch está deployada em https://tabnews-git-userprovider-tabnews.vercel.app/ para testarmos 👍

@filipedeschamps
Copy link
Owner

@aprendendofelipe veja se você concorda com o que eu fiz no commit b924f20 se sim, me sinto pronto para merge na main 🤝

@aprendendofelipe
Copy link
Collaborator Author

@aprendendofelipe veja se você concorda com o que eu fiz no commit b924f20 se sim, me sinto pronto para merge na main 🤝

Boraaaaaa!!!!

@aprendendofelipe
Copy link
Collaborator Author

Acho que esse PR vai ficar aberto até a fase 2, né? Se for, o que acha da seguinte mudança?

Fazer o useUser retornar username, user_id e user_features separadamente do user.

Acredito que assim o React irá lidar melhor com a mudança do user do cache pelo revalidado e vai evitar o render que ocorre nessa hora.

@filipedeschamps filipedeschamps merged commit 35db16c into main Jun 14, 2022
@filipedeschamps filipedeschamps deleted the UserProvider branch June 14, 2022 02:07
@filipedeschamps
Copy link
Owner

Merged! Let's goooooo!!!!!

@filipedeschamps
Copy link
Owner

Acho que esse PR vai ficar aberto até a fase 2, né? Se for, o que acha da seguinte mudança?

Fazer o useUser retornar username, user_id e user_features separadamente do user.

Acredito que assim o React irá lidar melhor com a mudança do user do cache pelo revalidado e vai evitar o render que ocorre nessa hora.

Opa, acabamos enviando na mesma hora acho 👍

Eu preciso fazer o merge para ele entrar em produção 🤝

E não entendi a otimização, por favor passar mais detalhes, pois pareceu interessante 😍

@aprendendofelipe
Copy link
Collaborator Author

E não entendi a otimização, por favor passar mais detalhes, pois pareceu interessante 😍

O React compara os objetos usando Object.is(a, b) para ver se precisa renderizar novamente o componente. Com isso, ao compararmos dois objetos user que tem as propriedades username, id e features idênticas, teoricamente não precisamos renderizar novamente, pois nada vai mudar visualmente, mas como são dois objetos diferentes, vai haver um novo render. Se estivéssemos comparando os primitivos (username, id e features), não haveria esse render extra.

Qualquer coisa abro outro PR depois para testar isso.

@filipedeschamps
Copy link
Owner

@aprendendofelipe show! Isso não tira aquele layout shift que acontece de carregar o box do editor né? Eu vejo que no playground do ByteMD isso não acontece: https://bytemd.js.org/playground/

Mas zero urgência nisso e também não sei se vale a pena fazer essa refatoração agora. Acho tão mais confortável acessar tudo pelo user, em paralelo já queria hoje começar a me focar nas tabcoins 💪 👍

@aprendendofelipe
Copy link
Collaborator Author

Isso não tira aquele layout shift que acontece de carregar o box do editor né? Eu vejo que no playground do ByteMD isso não acontece: https://bytemd.js.org/playground/

Nossa! Não tinha reparado nisso. Não tem relação, não. O que acontece é que não está vindo o HTML do Editor na página estática. Então o editor só aparece no segundo render e desloca o resto pra baixo.

Depois vou investigar o motivo, mas é mais um ponto contra o ByteMD, pois também tem alguns problemas com responsividade em dispositivos móveis. Esse problema dá pra ver no playground. Você digita e o cursor sai do campo de visão (#387) e, as vezes, o cursor muda de posição, indo para o meio do texto já digitado.


Acho tão mais confortável acessar tudo pelo user

Bom, depois vou testar de qualquer jeito, porque não aguento a curiosidade. 😅 Só se eu ver que dá diferença significativa, mando um PR, mas mantendo o objeto user também, para poder ser usado onde fizer mais sentido.


em paralelo já queria hoje começar a me focar nas tabcoins 💪 👍

🚀🚀🚀

@filipedeschamps
Copy link
Owner

@aprendendofelipe vamos partir para o estagio 2?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
front Envolve modificações no frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants