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

Do not use inline scripts #173

Merged
merged 6 commits into from
May 26, 2020
Merged

Do not use inline scripts #173

merged 6 commits into from
May 26, 2020

Conversation

palant
Copy link
Collaborator

@palant palant commented May 22, 2020

The first commit here moves loading JavaScript to the top of the page, which requires putting lots of code into a DOMContentLoaded event listener - this code originally expected all the elements to be present already. The theme switcher's inline script has been removed (fixes #145 for me).

I will add more commits to get rid of other inline scripts as well.

@palant palant marked this pull request as draft May 22, 2020 07:40
@palant
Copy link
Collaborator Author

palant commented May 22, 2020

Extensive changes to script.html in the latest commit:

  • Not using .Scratch unnecessarily any more.
  • Scripts are processed only when needed rather than always at the top of the template.
  • Moved the generic part of script processing into a template to avoid repeating the code.

Note that I renamed $Deliver into $ for this template - it should be more obvious this way. What do you think, maybe do it similarly in other templates as well?

@reuixiy
Copy link
Owner

reuixiy commented May 22, 2020

What do you think, maybe do it similarly in other templates as well?

Wow! That would be great!

@palant
Copy link
Collaborator Author

palant commented May 22, 2020

Not using .Scratch unnecessarily any more.

I messed things up here. Scratch is actually necessary to get scripts from other templates, e.g. lunr-search.html. But maybe we should use a private scratch rather than a global one...

@palant
Copy link
Collaborator Author

palant commented May 22, 2020

This is a lot more messy than I expected, with those third-party scripts expecting to be loaded at a particular time...

@palant
Copy link
Collaborator Author

palant commented May 22, 2020

I guess I'll keep the various third-party scripts unchanged at the bottom and only fix the built-in functionality.

@palant
Copy link
Collaborator Author

palant commented May 25, 2020

Third-party scripts are now in a separate template, back at the bottom of the page - no functionality change here. This allowed making the logic for the built-in scripts simpler again.


{{- .Scratch.Add "script" (slice $custom) -}}
{{- $processedScripts := slice ("" | resources.FromString "dummy.js") -}}
Copy link
Collaborator Author

@palant palant May 25, 2020

Choose a reason for hiding this comment

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

This is a weird hack but it cannot be an empty slice. Apparently, slices are typed and an empty slice will be created with the wrong type making resources.Concat fail.

@palant palant marked this pull request as ready for review May 25, 2020 20:27
@palant palant requested a review from reuixiy May 25, 2020 20:27
@palant palant merged commit f160158 into master May 26, 2020
@palant palant deleted the no-inline-scripts branch May 26, 2020 05:11
@palant
Copy link
Collaborator Author

palant commented May 26, 2020

Somehow I only noticed a bug after uploading this to a real website, it wasn't visible in local testing: getCurrentTheme() would error out because styles didn't load yet and the property it attempted to parse didn't exist. I pushed e08e25d to fix this.

@reuixiy reuixiy mentioned this pull request Jun 14, 2020
ulmefors pushed a commit to ulmefors/hugo-theme-meme that referenced this pull request Nov 22, 2020
* fix: Move loading JavaScript to the top of the page, don't use inline scripts for the theme switcher (fixes reuixiy#145)

* fix: Do not use an inline script for HTTPS redirect

* chore: Actually remove the inline script from page head

* chore: Use scratch again (a private one) and fix lunr search

* fix: Move third-party scripts back to the bottom of the page

BREAKING CHANGE: custom.js is loaded at the top of the page now, before the page elements are available
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.

Inline scripts break Content Security Policy
2 participants