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

Separate NexT inline scripts and styles #226

Merged
merged 10 commits into from
Apr 1, 2021

Conversation

PaperStrike
Copy link
Member

@PaperStrike PaperStrike commented Mar 10, 2021

PR Checklist

  • The commit message follows guidelines for NexT.
  • Tests for the changes was maked (for bug fixes / features).
    • Muse | Mist have been tested.
    • Pisces | Gemini have been tested.
  • Docs in NexT website have been added / updated (for features).

PR Type

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

What is the current behavior?

NexT is using several inline scripts and inline styles.

Issue resolved: #220

What is the new behavior?

Scripts (except third party and schedule modules) will be load in separate files. (Demo site now runs without setting unsafe-inline in script-src of CSP.)
Inline styles remains where they are for now, since they are a little bit scattered.
But all of them will be separated in future commits.

Tests is not enough yet.

  • Link to demo site with this changes: sliphua.work
  • Screenshots with this changes:

How to use?

In NexT _config.yml:

- TODO: Compatibility with Pjax.
- Use <meta name="hexo-config[-page]"> to store configurations
- Combine js scripts in `head.njk` and `head-unique.njk` into `load-config.js`
- Remove unused class `hexo-configurations`
- Background image of `.link-grid-image` are defined by passing parameters to and then generated by `link-grid.js`, which means it can't be moved into single CSS file directly. A more proper way would be changing the type of it, from `<div>` to `<img>`, and use `src` instead of `style="background-image: "` - which needs a discussion
@github-actions github-actions bot added the CSS label Mar 10, 2021
@PaperStrike
Copy link
Member Author

I separated most of the inline styles in the last commit. There are one inline style I left untouched in scripts/tags/link-grid.js.

Background image of .link-grid-image are defined by passing parameters to and then generated by link-grid.js, which means it can't be moved into a single CSS file directly. A more proper way would be changing the type of it, from <div> to <img>, and use src instead of style="background-image: ".

By changing the type, not only the related CSS code in NexT needs changes, styles defined by users may also be affected. So, should I leave the way it is or change its type and related CSS anyway?

@PaperStrike PaperStrike marked this pull request as ready for review March 10, 2021 12:51
- Refactor `load-config.js` and `pjax.js` to load and update all configurations at once.
@PaperStrike
Copy link
Member Author

Scripts have (almost) all been separated into .js files, except the third-party ones.
Styles have (almost) all been separated into .styl files, except the link-grid one.

@stevenjoezhang What's our next move? ("Except" the "except"s, or leave the way they are?)

layout/_layout.njk Outdated Show resolved Hide resolved
@PaperStrike PaperStrike force-pushed the csp-no-unsafe-inline branch from 08f5e7a to 0e18826 Compare March 11, 2021 08:13
@PaperStrike PaperStrike added this to the 8.3.0 milestone Mar 11, 2021
@PaperStrike
Copy link
Member Author

PaperStrike commented Mar 14, 2021

Edit: see my next comment and ignore this.


@next-theme/core I am trying to separate the scripts in layout/_third-party/*[/*]/*.njk, and noticed that a lot of them creates a <script> and injects to <head>. Like:

layout/_third-party/analytics/baidu-analytics.njk:

{%- if theme.baidu_analytics %}
  <script{{ pjax }}>
    var _hmt = _hmt || [];
    (function() {
      var hm = document.createElement("script");
      hm.src = "https://hm.baidu.com/hm.js?{{ theme.baidu_analytics }}";
      var s = document.getElementsByTagName("script")[0];
      s.parentNode.insertBefore(hm, s);
    })();
  </script>
{%- endif %}

I don't understand. What's the difference it made from:

{%- if theme.baidu_analytics %}
  <script{{ pjax }}>var _hmt = _hmt || [];</script>
  <script{{ pjax }} src="https://hm.baidu.com/hm.js?{{ theme.baidu_analytics }}"></script>
{%- endif %}

?
When browser parser reach these lines, the parsed has been parsed, injects new script to <head> will not change the execute order. it will turn to run the just-injected after the current script is finished, which is the same as the second code block.

If there is something I missed out, appeciate you informing me. or I may refactor some parts of the scripts like this.

@jiangtj
Copy link

jiangtj commented Mar 15, 2021

baidu-analytics, this part of the code may be copied directly from Baidu

@PaperStrike
Copy link
Member Author

PaperStrike commented Mar 15, 2021

@next-theme/core Ignore my last commit and take a look at this one 😅, I figured out a new way to implement the third party scripts, which is to edit and move the .njk files to .js files (layout/_third-party[/*]/*.njk -> source/js/third-party[/*]/*.js, except every index.njk). I believe it's better for performance and future coding.

There is a possible sample, it

  • calls CONFIG.getCurrent() to get parameters.

    // CONFIG.getCurrent() returns the parsed `data-config` of
    // `document.currentScript`.
    const disqusConfig = CONFIG.getCurrent();
  • listens topage:ready event

    // `page:ready` is fired when new configurations have been set
    // after `DOMContentLoaded` and each `pjax:success`.
    document.addEventListener('page:ready', () => {

    instead of executed by

    pjax.executeScripts(document.querySelectorAll('script[data-pjax], .pjax script'));

  • uses NexT.utils.getScript to conditionally inject scripts.

    // `NexT.utils.getScript` now recieves a fourth
    // parameter as the script's attributes.
    // Use `NexT.utils.getScript` instead of creating a
    // `<script>` here, so that we can easily refactor
    // when there is a better way.
    NexT.utils.getScript(
      `https://${disqusConfig.shortname}.disqus.com/count.js`,
      () => {},
      false,
      { id: 'dsq-count-scr' }
    );

Most of the third party scripts are injected once it is enabled, while the ones about comments (layout/_third-party/comments/*) (or more) are injected when it is enabled and page.comments evaluates to true(, there may be more conditions to fit but they're similar here). As page.comments differs in different pages, you may complain that the scripts may be loaded but unneeded.

But it's easy to write some conditions for the pages that don't use Pjax, to not implement these .js when corresponding additional conditions evaluates to false, the same way we do in current .njk. So, the only case where these scripts are "loaded but unneeded" will be when the site uses Pjax and the user did not open any page that fits the condition. Acturally, it boosts the speed of the sites with Pjax enabled as these scripts will not be reinjected by pjax.executeScripts, does no harm to sites with Pjax disabled.

Feel free to share your opinions.

@PaperStrike
Copy link
Member Author

PaperStrike commented Mar 15, 2021

If we are lucky, changing the type of .link-grid-image (0cc96fc) won't cause any problems. I have tested it in my browsers, comparing the original <div> one and <object> one in a same page, and noticed nothing visible is changed after changing the type and editing related CSS properties. But the compatibility remainds to be seen, especially that someone would have written some custom CSS code about it.

I decided to use <object> instead of <img> to prevent the broken image being shown when the image fails to load. Besides, with <object>, we will be able to customize the HTML content when it fails, as the standard describes: (fallback here is like onerror, click the link for details.)

  1. Fallback: The object element represents the element's children, ignoring any leading param element children. This is the element's fallback content. If the element has an instantiated plugin, then unload it. If the element's nested browsing context is non-null, then it must be discarded and then set to null.

If you are fine with these changes, this pull request will be merged very soon, and future discussions about the third-party implements will continue in a new pull request.

@stevenjoezhang stevenjoezhang modified the milestones: 8.3.0, 8.4.0 Mar 31, 2021
@stevenjoezhang stevenjoezhang merged commit 6b23d42 into next-theme:master Apr 1, 2021
@PaperStrike PaperStrike deleted the csp-no-unsafe-inline branch April 1, 2021 11:54
@PaperStrike PaperStrike restored the csp-no-unsafe-inline branch April 4, 2021 09:25
@PaperStrike PaperStrike mentioned this pull request Apr 5, 2021
12 tasks
ljcbaby pushed a commit that referenced this pull request Apr 5, 2021
* Fix head inject point

* Switch `.link-grid-image` to `<img>`

* Switch `.link-grid-image` back to `<object>`

- Set `type` to `image/jpeg` for image without file extension
@PaperStrike PaperStrike mentioned this pull request Apr 5, 2021
29 tasks
@PaperStrike PaperStrike changed the title Separate inline scripts and styles into single files Separate NexT inline scripts and styles Apr 20, 2021
lingyf pushed a commit to lingyf/hexo-theme-next that referenced this pull request Jan 27, 2022
lingyf pushed a commit to lingyf/hexo-theme-next that referenced this pull request Jan 27, 2022
* Fix head inject point

* Switch `.link-grid-image` to `<img>`

* Switch `.link-grid-image` back to `<object>`

- Set `type` to `image/jpeg` for image without file extension
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.

移除对 CSP 响应头 'unsafe-inline' 的依赖
4 participants