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

Diminuir tamanho do JavaScript inicial da página #1063

Merged
merged 2 commits into from
Dec 9, 2022
Merged

Conversation

aprendendofelipe
Copy link
Collaborator

@aprendendofelipe aprendendofelipe commented Dec 1, 2022

Com o PR #1061 o first load passou de 450 kB para 728 kB apenas por utilizar algumas funções da biblioteca highlight.js (listLanguages() e getLanguage()).

Como o retorno dessas funções é estático (poderá mudar apenas com a atualização da biblioteca), foi criada uma constante com o valor retornado (lista de linguagens compatíveis com o highlight) e o código que chama as funções foi mantido de maneira comentada para permitir a geração automatizada de nova constante em caso de necessidade futura.

E com isso o first load voltou para o patamar anterior, ficando um pouco menor que antes, com 443 kB.

No segundo commit foi adicionado um ordenamento final na consulta da lista de conteúdos para garantir sempre o mesmo retorno e parar de gerar alguns erros aleatórios nos testes (como ocorrido aqui #910 (comment) e aqui #1012 (comment)).

@vercel
Copy link

vercel bot commented Dec 1, 2022

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

Name Status Preview Updated
tabnews ✅ Ready (Inspect) Visit Preview Dec 8, 2022 at 9:13PM (UTC)

@@ -47,6 +47,7 @@ async function findAll(values = {}, options = {}) {
${values.count ? 'LIMIT 1' : 'LIMIT $1 OFFSET $2'}
)
${selectClause}
${values.count ? '' : orderByClause}
Copy link
Owner

Choose a reason for hiding this comment

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

Me ajuda, estou tentando entender o que isso faz e não consigo 🤝

Se tem count, a janela de conteúdos não é ordenada, mas se tem, ela é ordenada pelo orderByClause. Não consigo entender a semântica disso.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Analisando pela linha 47, se tem values.count, apenas um elemento é retornado (LIMIT 1), então não tem necessidade de ordenar.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

É verdade que com o LIMIT 1 não há necessidade de ordenar, mas não é por isso que eu adicionei esse condicional.

Quando tem count, a selectClause não faz JOIN (porque não precisamos de nada além do total_rows), então gera erro se tentarmos ordenar por colunas que não estão disponíveis.

Mas o fato que vocês bem lembraram, de que não precisamos ordenar quando tem o count, é legal, pois a gente pode tirar o ordenamento também da linha 45.

E daí pode ficar melhor já verificar isso dentro de buildOrderByClause, então vou mudar 👍

@aprendendofelipe
Copy link
Collaborator Author

@filipedeschamps, fora o first load que volta pra um patamar bem melhor, com a alteração da query não vai mais dar o erro aleatório que acabou de se repetir no teste do PR #804.

Veja na prática a diferença de ordenação dos conteúdos:

Correto: https://tabnews-git-app-size-tabnews.vercel.app/FelipeBarso

Aleatório: https://tabnews-git-refresh-user-tabnews.vercel.app/FelipeBarso

@filipedeschamps filipedeschamps self-requested a review December 8, 2022 22:20
@filipedeschamps
Copy link
Owner

@aprendendofelipe sensacional!!! E a sua ajuda com os PRs ta sendo FENOMENAL!!!!

@aprendendofelipe aprendendofelipe merged commit 0800a64 into main Dec 9, 2022
@aprendendofelipe
Copy link
Collaborator Author

@aprendendofelipe sensacional!!! E a sua ajuda com os PRs ta sendo FENOMENAL!!!!

Amanhã vejo mais alguns 👍🚀

@aprendendofelipe aprendendofelipe deleted the app-size branch December 9, 2022 01:38
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.

4 participants