podcodar / voting-system

VS
voting-system-lyart.vercel.app
1 stars 0 forks source link

feat collect and persist notion api key with cookies #28

Closed vzsoares closed 2 years ago

vzsoares commented 2 years ago

Description

Changes

Notes

Board issue

vercel[bot] commented 2 years ago

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

Name Status Preview Updated
voting-system ✅ Ready (Inspect) Visit Preview Aug 12, 2022 at 2:20PM (UTC)
frattezi commented 2 years ago

Sobre enviar os cookies na request, o browser vai fazer isso automaticamente de acordo com o domain onde aqueles cookies estão cadastrados.

Testando sua branch, uma vez que o cookie está cadastrado ele está sendo enviado.

Você pode ver isto na aba de Network -> qualquer requição para a API -> request headers

Screenshot from 2022-07-29 16-01-01

Tem mais infos na MDN docs.

vzsoares commented 2 years ago

Sobre enviar os cookies na request, o browser vai fazer isso automaticamente de acordo com o domain onde aqueles cookies estão cadastrados.

Testando sua branch, uma vez que o cookie está cadastrado ele está sendo enviado.

Você pode ver isto na aba de Network -> qualquer requição para a API -> request headers

Tem mais infos na MDN docs.

@frattezi o problema esta sendo no arquivo client.ts , é preciso criar esse Client mas quando tento pegar o cookie dá undefined no servidor , mas no log do navegador aparece

frattezi commented 2 years ago

Sobre enviar os cookies na request, o browser vai fazer isso automaticamente de acordo com o domain onde aqueles cookies estão cadastrados. Testando sua branch, uma vez que o cookie está cadastrado ele está sendo enviado. Você pode ver isto na aba de Network -> qualquer requição para a API -> request headers

Tem mais infos na MDN docs.

@frattezi o problema esta sendo no arquivo client.ts , é preciso criar esse Client mas quando tento pegar o cookie dá undefined no servidor , mas no log do navegador aparece

Vou olhar mais a fundo hoje man, na minha cabeça já era para funcionar

frattezi commented 2 years ago

Sobre enviar os cookies na request, o browser vai fazer isso automaticamente de acordo com o domain onde aqueles cookies estão cadastrados. Testando sua branch, uma vez que o cookie está cadastrado ele está sendo enviado. Você pode ver isto na aba de Network -> qualquer requição para a API -> request headers

Tem mais infos na MDN docs.

@frattezi o problema esta sendo no arquivo client.ts , é preciso criar esse Client mas quando tento pegar o cookie dá undefined no servidor , mas no log do navegador aparece

Man ainda não entendi oque você quis dizer. Sim, o browser demonstra por que a request possui os cookies no header, não existe nada, que possa interceptar ou modificar os cookies nesse meio tempo, logo ele são acessíveis sim pela api.

Fiz uma run simples, adicionei a chamada de getElections para ser feita no load da página index da aplicação, no endpoint da API fiz:

pages/api/elections/index.ts

export default async function handler(
  req: NextApiRequest,
  res: NextApiResponse<GetAvaiableElectionsResponse>,
) {
  const { databaseId } = req.query;
  const { cookie } = req.headers;
  console.log(cookie);

Quando a API recebeu a requisição, o log foi:

notionApiKey=testes #key de teste que setei

O que quero dizer é, não existe problema no client, cookies são enviados diretamente no cabeçalho da requisição pelo browser a depender da configuração de path que o cookie foi salvo.

O PR está bom, peço que por favor confira se já temos uma task na board relacionada a utilizarmos a notion api key via cookies, se não tiver poderia cadastrar por favor?

E apenas antes de fazer o merge, poderia considerar meu comment sobre não inserir uma lib para cookies? Acho que podemos apenas fazer 2 functions get e set e usálas pra semrpe sem nunca mais pensar, ao invés de adicioonar code terceiro na base. Veja este exemplo, me parece uma implementação legalzinha.

vzsoares commented 2 years ago

a questao ali é q o notion apiKey nao é utilizado nesse arquivo e sim no , notion/client.ts onde ele ta pegando do prosses.env mas do cookie nao consegui pegar ali

frattezi commented 2 years ago

a questao ali é q o notion apiKey nao é utilizado nesse arquivo e sim no , notion/client.ts onde ele ta pegando do prosses.env mas do cookie nao consegui pegar ali

Agora acho que entendi o contexto todo. Realmente vai mudar um bocado nossos fluxos, mas é algo que iamos ter de pensar em algum momento.

Basicamente, estávamos instanciando o NotionClient apenas 1 vez durante o run da aplicação. Porém, nesta modificação (que é como o app tem de funcionar, diversos clientes com diversas chaves notion).

Penso que, é possível resolver isto agora melhorando um pouco nosso cliente notion, poderíamos seguir um formato de fazer um singleton para este cara.

Podemos fazer uma nova classe NotionClient, esta classe é dedicada a instanciar e centralizar a comunicação com o Notion API.

Se seguirmos um padrão próximo de um Singleton, esta class poderia expor um método público, getNewClient(notionApiKey: string) Este método é encarregado de instanciar o notionClient pra gente caso, já não exista, a depender da key dada pelo usuário.

Assim seria possível gerenciarmos diferentes clientes de notion, permitindo a execução da aplicação simultânea. Me parece um code massa de fazer, podemos fazer um pair ou sentar para discutir se você quiser

vzsoares commented 2 years ago

Parabéns pelo PR, acho que a solução ficou bem legal.

Sobre o problema no server, deixei um comment para uma solução massa de implementar também, podemos discutir mais mas acredito que foge ao escopo deste PR.

pra essa issue esse pr creio q ja basta, mas temos a issue #20 que seria exatamente isso q falou. Bora planejar uma soluçao para ela dps sim.

frattezi commented 2 years ago

@vzsoares ja podemos mergear este aqui?

vzsoares commented 2 years ago

@vzsoares ja podemos mergear este aqui?

image

frattezi commented 2 years ago

Essa rule de 2 review não vai funfar com o time reduzido e galera apertada no trampo, reduzi para 1

My bad não tinnha visto, pode seguir