Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

Fix Lighthouse errors #200

Merged
merged 5 commits into from
Apr 14, 2022
Merged

Fix Lighthouse errors #200

merged 5 commits into from
Apr 14, 2022

Conversation

aabounegm
Copy link
Owner

@aabounegm aabounegm commented Apr 14, 2022

I enabled the CSP again but disabled the strict-dymanic source since SvelteKit doesn't support it properly yet (sveltejs/kit#3558).
As for the unused-javascript issue, I disabled the assertion since it is mostly not in our control (and SvelteKit basically closed the issue as wontfix - sveltejs/kit#1371). It might be possible to work around it (according to this Reddit comment), but it feels like a dirty hack, so I'd say it's good as it is now.

Resolves #167

Added the 'self' source and removed 'https' and 'strict-dynamic'
'https' would allow scripts from any HTTPS source
and 'strict-dynamic' doesn't work with SvelteKit yet
@github-actions github-actions bot requested a review from khaledismaeel April 14, 2022 09:10
@aabounegm aabounegm enabled auto-merge April 14, 2022 09:19
Copy link
Collaborator

@illright illright left a comment

Choose a reason for hiding this comment

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

praise: thank you so much for taking this off of me and figuring out the CSP issue :)

src/app.html Outdated
@@ -4,6 +4,7 @@
<meta charset="utf-8" />
<meta name="description" content="Listen to podcasts on the web, wherever!" />
<link rel="manifest" href="%svelte.assets%/manifest.json" />
<link rel="preconnect" href="https://sklgqhbbbhticzdgegcj.supabase.co" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

issue: I don't think we should hard-code this

suggestion: in the app layer, there are a few components that manage meta tags, perhaps you could move it there and use the value from the environment variable?

To use the environment variable
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Apr 14, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 0591154
Status: ✅  Deploy successful!
Preview URL: https://d49ed48d.cast-iu.pages.dev

View logs

@codecov-commenter
Copy link

codecov-commenter commented Apr 14, 2022

Codecov Report

Merging #200 (0591154) into main (a982402) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #200   +/-   ##
=======================================
  Coverage   71.54%   71.54%           
=======================================
  Files          66       66           
  Lines         629      629           
  Branches      162      162           
=======================================
  Hits          450      450           
  Misses        167      167           
  Partials       12       12           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a982402...0591154. Read the comment docs.

@aabounegm aabounegm merged commit 5cbc1fd into main Apr 14, 2022
@aabounegm aabounegm deleted the fix/lighthouse branch April 14, 2022 09:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix performance testing
3 participants