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

Adding Submodule #663

Closed
wants to merge 3 commits into from

Conversation

@stevenjoezhang
Copy link
Member

stevenjoezhang commented Mar 7, 2019

Breaking Change:

Install new modules:

git submodule init fancybox pangu
git submodule update

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines.
  • 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 new features).

PR Type

What kind of change does this PR introduce?

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

What is the current behavior?

Issue resolved: #413

What is the new behavior?

  • Screenshots with this changes: N/A
  • Link to demo site with this changes: N/A

How to use?

In NexT _config.yml:

...

Does this PR introduce a breaking change?

  • Yes.
  • No.
Update
@1v9

This comment has been minimized.

Copy link
Member

1v9 commented Mar 7, 2019

fancybox3 & gitment?

@stevenjoezhang

This comment has been minimized.

Copy link
Member Author

stevenjoezhang commented Mar 7, 2019

Gitment is not designed to be cloned. It just provides a jsDelivr CDN, because the origin Gitment repo is not maintained
fancybox3 and fancybox both uses /source/lib/fancybox, to avoid potential conflict, I choose fancybox3

.gitmodules Outdated Show resolved Hide resolved
@stevenjoezhang stevenjoezhang requested a review from 1v9 Mar 7, 2019
@1v9

This comment has been minimized.

Copy link
Member

1v9 commented Mar 7, 2019

Need update much docs README...

@1v9 1v9 self-requested a review Mar 7, 2019
@1v9 1v9 dismissed their stale review Mar 7, 2019

tarvis-ci fasiled

@1v9

This comment has been minimized.

Copy link
Member

1v9 commented Mar 7, 2019

@stevenjoezhang

This comment has been minimized.

Copy link
Member Author

stevenjoezhang commented Mar 7, 2019

Seems tarvis-ci does not know how to clone a repo with ssl

@ivan-nginx

This comment has been minimized.

Copy link
Member

ivan-nginx commented Mar 7, 2019

Don't totally understand: for what this needed?


Also, Travis make error on 'source/lib/fancybox' module.

@jiangtj

This comment has been minimized.

Copy link
Member

jiangtj commented Mar 7, 2019

Don't totally understand: for what this needed?

Confusion too. Submodule can be done by user.

@stevenjoezhang

This comment has been minimized.

Copy link
Member Author

stevenjoezhang commented Mar 7, 2019

This allows users installing dependencies using a simple command, no longer need to check and follow the instructions in README.md of these ‘theme-next-*’ repos

@jiangtj

This comment has been minimized.

Copy link
Member

jiangtj commented Mar 7, 2019

This allows users installing dependencies using a simple command, no longer need to check and follow the instructions in README.md of these ‘theme-next-*’ repos

Some GUI tools would clone all submodule when once clone, such as GitHub desktop......
But I don't use all.

@LEAFERx

This comment has been minimized.

Copy link
Contributor

LEAFERx commented Mar 7, 2019

Personally, instead of git submodule, I prefer that plugins be hexo plugins (npm packages), and insert to theme using hexo tags.
For example, if you want jquery, you can create a plugin that provides {% jquery-script %} called hexo-jquery and users can just simply install it and change _config.yml to set useJquery: Ture.
It is convenient for users and can be simply reused by other themes.
Think about that lol. Why a theme plugin is not a hexo plugin?

@LEAFERx

This comment has been minimized.

Copy link
Contributor

LEAFERx commented Mar 7, 2019

@ivan-nginx

This comment has been minimized.

Copy link
Member

ivan-nginx commented Mar 7, 2019

Guys, check here before resume discussion of this PR.

@LEAFERx

This comment has been minimized.

Copy link
Contributor

LEAFERx commented Mar 7, 2019

Guys, check here before resume discussion of this PR.

Yes it's a great view. And automatic download may be easier to be done by npm packages lol.

@1v9

This comment has been minimized.

Copy link
Member

1v9 commented Mar 7, 2019

Philosophical debate is coming🤔. I think if there's not a good solution just don't change anything.

@ivan-nginx

This comment has been minimized.

Copy link
Member

ivan-nginx commented Mar 7, 2019

automatic download may be easier to be done by npm packages

There is some problems with it:

  1. node_modules path is different from themes path. But it's not critical and may be solved, of course.
  2. For now NexT used Netlify for deploy and each time during generate Netlify reinstall node_modules, but not NexT theme. See logs here and here, for example. In 1st logs NexT just updated needed files; in 2nd – don't touch because «Already up to date». I'm talking about traffic, Netlify give 100GB per month.

But maybe node_modules cached somehow, I'm not sure. However, as an option we can try to move modules from Git repos to NPM and check it for traffic.


BTW, if we talking about NPM, maybe NexT can live on NPM too? But it's a little bit extra work: make release on Git, then make release on NPM. Need to think about it.

@jiangtj

This comment has been minimized.

Copy link
Member

jiangtj commented Mar 7, 2019

About npm.
I think the main problem is swig. Something written in swig file. How to solved it?

@ivan-nginx

This comment has been minimized.

Copy link
Member

ivan-nginx commented Mar 7, 2019

What swig? NPM can include any files. Moreover, almost all functions can be moved from swig to js files in scripts directory. But on it will wasted some time, of course.

@jiangtj

This comment has been minimized.

Copy link
Member

jiangtj commented Mar 7, 2019

Such as comments:

{% if theme.disqus.enable %}
  {% include 'disqus.swig' %}
{% elif theme.changyan.enable and theme.changyan.appid and theme.changyan.appkey %}
......

We need config in comment index.swig, If we use npm, need wasted some time to add code in the right place. But it's not easy.

@jiangtj

This comment has been minimized.

Copy link
Member

jiangtj commented Mar 7, 2019

100GB per month

Only when a user visits a page will it consume. So don't worry reinstall node_modules.

@ivan-nginx

This comment has been minimized.

Copy link
Member

ivan-nginx commented Mar 7, 2019

We need config in comment index.swig, If we use npm, need wasted some time to add code in the right place.

Maybe. But u talking now about NexT. Need to make decision about modules firstly.

Only when a user visits a page will it consume

Yeah? Are u sure downloading packages / repos (deploying) not waste this traffic?


For now: 382 MB/100 GB. It's 7 days usage.

@LEAFERx

This comment has been minimized.

Copy link
Contributor

LEAFERx commented Mar 7, 2019

By hexo plugins I mean install in root of the blog, not in NexT.
I'm working on the leancloud conuter refactor including modulize and automatic setup. I can show you how I design the module to work when I'm finished. Hope I can finish before next week.

Such as comments:

{% if theme.disqus.enable %}
  {% include 'disqus.swig' %}
{% elif theme.changyan.enable and theme.changyan.appid and theme.changyan.appkey %}
......

We need config in comment index.swig, If we use npm, need wasted some time to add code in the right place. But it's not easy.

Sure current code need refactor. But we can do it step by step and still keep those small pieces inside. And automatic install dependencies for NexT does excite me a lot.

automatic download may be easier to be done by npm packages

There is some problems with it:

  1. node_modules path is different from themes path. But it's not critical and may be solved, of course.
  2. For now NexT used Netlify for deploy and each time during generate Netlify reinstall node_modules, but not NexT theme. See logs here and here, for example. In 1st logs NexT just updated needed files; in 2nd – don't touch because «Already up to date». I'm talking about traffic, Netlify give 100GB per month.

But maybe node_modules cached somehow, I'm not sure. However, as an option we can try to move modules from Git repos to NPM and check it for traffic.

BTW, if we talking about NPM, maybe NexT can live on NPM too? But it's a little bit extra work: make release on Git, then make release on NPM. Need to think about it.

Netlify do cache npm modules. see here

@1v9 1v9 removed their request for review Mar 9, 2019
@LEAFERx LEAFERx referenced this pull request Mar 11, 2019
6 of 14 tasks complete
@ivan-nginx ivan-nginx referenced this pull request Mar 24, 2019
42 of 72 tasks complete
@ivan-nginx ivan-nginx reopened this Apr 2, 2019
@ivan-nginx ivan-nginx added this to In progress in Feats. / Imprs. / Fixes via automation Apr 2, 2019
Feats. / Imprs. / Fixes automation moved this from In progress to Done May 3, 2019
@ivan-nginx ivan-nginx removed this from Done in Feats. / Imprs. / Fixes May 3, 2019
@stevenjoezhang stevenjoezhang reopened this May 3, 2019
@ivan-nginx ivan-nginx referenced this pull request Jul 17, 2019
16 of 36 tasks complete
@stevenjoezhang stevenjoezhang referenced this pull request Jul 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.