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

perf: Optimize js file loading condition and order #683

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kemchenj
Copy link

@kemchenj kemchenj commented Aug 7, 2023

PR Checklist

PR Type

  • Bugfix.
  • Feature.
  • Improvement.
  • Code style update (formatting, linting).
  • Refactoring (no functional changes).
  • Documentation.
  • Translation.
  • Other... Please describe:

What is the current behavior?

Issue resolved:

  • The source/js/comments.js will included in html even if none of the comment systems is enabled.
  • The source/js/config.js is inside the head tag, which will block the rendering, but it doesn't have to.

What is the new behavior?

  • The source/js/comments.js will not be included in html if none of the comment system is using.
  • The source/js/config.js is now at the end of the body tag, right after the vendor scripts and before all the next script, which won't block the DOM rendering and everything works just like before.

How to use?

Disable all the comment system or enable some of them in NexT _config.yml.

@welcome
Copy link

welcome bot commented Aug 7, 2023

Thanks so much for opening your first PR here!

@CLAassistant
Copy link

CLAassistant commented Aug 7, 2023

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

coveralls commented Aug 7, 2023

Pull Request Test Coverage Report for Build 6130948666

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 97.416%

Totals Coverage Status
Change from base Build 6128525535: 0.0%
Covered Lines: 394
Relevant Lines: 399

💛 - Coveralls

@kemchenj
Copy link
Author

kemchenj commented Aug 8, 2023

After waking up, I realized there was a small issue with the conditions for loading comments. Previously it was page.comments or theme.injects.comment.length > 0, but checking page.comments was unnecessary and incorrect, so I pushed an update to remove it.

Copy link
Member

@stevenjoezhang stevenjoezhang left a comment

Choose a reason for hiding this comment

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

@kemchenj
Copy link
Author

kemchenj commented Sep 9, 2023

Scripts like https://github.com/next-theme/hexo-theme-next/blob/master/source/js/third-party/analytics/google-analytics.js are loaded in <head> and requires CONFIG variable

Emm, didn't notice that, looks like it requires more changes than I thought before.

For now I will just delete the changes about config.js. Please have a look again.

@@ -1,6 +1,8 @@
{%- include 'vendors.njk' -%}

{{- next_js('comments.js') }}
{%- if theme.injects.comment.length > 0 %}
Copy link
Member

Choose a reason for hiding this comment

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

Actually, the condition could be '> 1' because if there's only one comment system, it doesn't have to switch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants