-
Notifications
You must be signed in to change notification settings - Fork 39
General code audit #34
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
Comments
@cwaring -- thank you for identifying any problems here as you find them! 🙏 I can separate into checklists or individual issues as need be. |
I'll report back with an evaluation within the next couple of weeks, and also my suggestions to #41. |
@zebateira - per our conversation, are you able to work with @cwaring and @jdiogopeixoto to get this and #41 resolved? As we discussed, we might also want to look into establishing some sort of ongoing instrumentation for the blog to keep an eye on performance as the blog continues to expand over time. Thank you! |
Note re instrumentation: @atopal recommends https://speedcurve.com/. |
Do we feel we can close this issue in favor of #41? |
@jessicaschilling I would prefer to go forward with a separate code audit from the payload issue. Will leave an update here soon about this. |
Agree with @zebateira here. There is a lot more to be covered than captured in that issue, best keep this on the radar. |
Initial list:
|
I'll maintain a list of items here as I notice them:
|
Added a few more items above :) |
@cwaring regarding the 404, is it possible to make it work on a subpath? It doesn't seem to be working: https://ipfs.fleek.co/ipfs/Qmb4soPiemEeQoYWciwsZXaCBP3AYeojjrCFdCGHjsSgwe/asdf Currently the script redirect to |
It's only possible by redirecting with the The advantage of using a 404 layout means it's easy to capture the url for the 404 and report to analytics. If we redirect to a root /404 page perhaps this is also possible by checking the referer? |
The alternative that would work in both scenarios might be: ipfs-404.html responsible for redirects -> {cid}/404 OR /404 Downside is this would only work if JS is enabled but who really browses with JS disabled these days?! |
I just checked, this would work but then you would also lose the original referer link because of the interim redirect. eg: This can be helpful for figuring out the original source link and fixing them. So you would have to pass on the original referer to the 404 page to capture everything correctly or do the analytics tracking before the redirect, but then you would have to wait for that to complete before redirecting to ensure it was captured. Simply using one 404.vue is the easiest solution for now. Also I know there aren't any 404 analytics yet on this site but that should be added asap :) |
WIP PR for 404 here: #106 (includes analytics as well :) ) |
@cwaring does this feel like it's complete? Or do you want another iteration after launch? |
It looks like we've covered all the burning issues now but we should do another round to actually check over optimisations and configuration, I haven't had time to go much deeper in this sprint. Calling this complete for now so we can move onto the pre-flight transition! |
This issue is for the need for an overall code audit, focusing in particular on:
Please separate the results out into their own issues so they're easier to work - thanks!
The text was updated successfully, but these errors were encountered: