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

Remove internal scripts if minify is true and the CDN provider is not local #251

Merged
merged 5 commits into from
Apr 30, 2021

Conversation

njzjz
Copy link
Member

@njzjz njzjz commented Apr 16, 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?

The local scripts are still remained if the CDN provider is not local. However, they will have never been used.

What is the new behavior?

Remove local scripts if minify is true and the CDN provider is not local.

How to use?

In NexT _config.yml:

minify: true
vendors:
   internal: jsdelivr  # or unpkg, cdnjs

(The current implement is a bit silly. Does some one have a better implement?)

The current implement is a bit silly. Does some one have a better implement?
@CLAassistant
Copy link

CLAassistant commented Apr 16, 2021

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

coveralls commented Apr 16, 2021

Coverage Status

Coverage increased (+0.06%) to 92.531% when pulling 42889e8 on njzjz:remove_js into c95976d on next-theme:master.

@njzjz njzjz changed the title Remove local scripts if the CDN provider is not local Remove local scripts if minify is true and the CDN provider is not local Apr 16, 2021
@PaperStrike
Copy link
Member

🤔 You yourself have a better implement in #241 (comment) . Like:

hexo.route.list().forEach(path => {
  if (path.startsWith('js/')) hexo.route.remove(path);
});

@njzjz
Copy link
Member Author

njzjz commented Apr 16, 2021

🤔 You yourself have a better implement in #241 (comment) . Like:

hexo.route.list().forEach(path => {
  if (path.startsWith('js/')) hexo.route.remove(path);
});

I am worried that this will break the plugins that add routes into the js directory (I am not sure if such the plugin exists).

@njzjz
Copy link
Member Author

njzjz commented Apr 21, 2021

Here is my solution:

Object.keys(hexo.theme._processingFiles).forEach(path => {
  if (path.startsWith('source/js/')) hexo.route.remove(path.slice(7));
});

@PaperStrike
Copy link
Member

PaperStrike commented Apr 22, 2021

Tracing how _processingFiles is generated, I found using addProcessor would be a better choice.

// ...

const localScripts = [];

hexo.theme.addProcessor('js/*', (file) => {
  localScripts.push(file.params[0]);
});

hexo.extend.filter.register('after_generate', () => {
  // ...

  if (theme.vendors.internal !== 'local') {
    localScripts.forEach(path => {
      hexo.route.remove(path);
    });
    return;
  }

  // ...
});

works fine.

@njzjz njzjz added this to the 8.4.0 milestone Apr 22, 2021
@njzjz
Copy link
Member Author

njzjz commented Apr 22, 2021

Tracing how _processingFiles is generated, I found using addProcessor would be a better choice.

Nice! It works on my blog.

@PaperStrike PaperStrike changed the title Remove local scripts if minify is true and the CDN provider is not local Remove internal scripts if minify is true and the CDN provider is not local Apr 30, 2021
@stevenjoezhang stevenjoezhang merged commit 959e100 into next-theme:master Apr 30, 2021
lingyf pushed a commit to lingyf/hexo-theme-next that referenced this pull request Jan 27, 2022
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.

5 participants