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

Enable to pull in docsy as hugo module #871

Merged
merged 7 commits into from
Feb 18, 2022
Merged

Enable to pull in docsy as hugo module #871

merged 7 commits into from
Feb 18, 2022

Conversation

deining
Copy link
Collaborator

@deining deining commented Jan 31, 2022

This is my final PR that enables users to pull in docsy as hugo module.
Having learned from the experiences that early adopters of docsy as hugo module made, I came up with the following solution:

We do have two modules now:

  • root module github.com/google/docsy this is the theme
  • submodule github.com/google/docsy/module, this is needed only if you use docsy as hugo modules, it pulls in dependencies bootstrap and Font-Awesome.

With all dependendies in place, one can now switch back and forth between traditional git submodules

theme = "docsy"

and modern docsy hugo module, pulled in from github:

theme = ["github.com/google/docsy", "github.com/google/docsy/module"]

Both modules were tagged as v0.2.0-pre and module/v0.2.0-pre resprectively, we should remove -pre once we merged this PR.

The corresponding documentation can be found in #802, it was updated and now reflect this latest state of module code as submitted with this PR.

This PR hopefully brings the module venture to a good end.

@LisaFC
Copy link
Collaborator

LisaFC commented Feb 2, 2022

This looks great! @chalin @emckean @geriom want to take a look?

@LisaFC
Copy link
Collaborator

LisaFC commented Feb 2, 2022

Will also need to review and potentially reorganize #802 when we're happy to add this to the main public docs.

Copy link
Collaborator

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Thanks for your efforts!
See inline comments and questions.

go.mod Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
userguide/config.toml Outdated Show resolved Hide resolved
module/config.toml Outdated Show resolved Hide resolved
config.toml Show resolved Hide resolved
@chalin
Copy link
Collaborator

chalin commented Feb 2, 2022

Thanks again for all your efforts @deining. I have a few questions -- see below.

  1. Is it correct to say that the Hugo module github.com/google/docsy/module contains docsy's dependencies, and only the dependencies (and the file workaround -- I like how we can localize the workaround)? If so, I think that a better name could be github.com/google/docsy/dependencies don't you think? Regardless, I think that to have a module named module doesn't feel right to me :) ... but that's a small detail.

  2. I don't know about Hugo modules or whether this is a good configuration. I know that you've been thinking about and experimenting with this for a while, but have you asked on the Hugo mailing list if this Hugo module config makes sense -- i.e., separating out a project's dependencies?

  3. My guess is that, some time in the future, if we decide that we no longer need to support docsy use via git submodules, then we'll be able to eliminate the dependencies module so that users can specify only theme = ["github.com/google/docsy"], is that correct?

  4. While this work in any way be impacted by Use NPM packages for Bootstrap and Font Awesome #870 if/when it gets resolved? (/cc @geriom)

  5. What are your thoughts on the relative ease (or not) of keeping Bootstrap and Fontawesome versions in sync across supported methods of using docsy (and the explicit assets imports in the files)? I'm asking because this PR introduces an extra set of dependencies to keep in sync with the others. (Maybe Use NPM packages for Bootstrap and Font Awesome #870 will make it a bit easier.)

That's it for now. Thanks for entertaining my questions.

@deining
Copy link
Collaborator Author

deining commented Feb 2, 2022

Thanks again for all your efforts @deining.

You are welcome.

  1. Is it correct to say that the Hugo module github.com/google/docsy/module contains docsy's dependencies, and only the dependencies (and the file workaround -- I like how we can localize the workaround)?

Yes.

If so, I think that a better name could be github.com/google/docsy/dependencies don't you think? Regardless, I think that to have a module named module doesn't feel right to me :) ... but that's a small detail.

I agree that github.com/google/docsy/module wasn't the best choice. Let me reflect on this a bit.

  1. I don't know about Hugo modules or whether this is a good configuration. I know that you've been thinking about and experimenting with this for a while, but have you asked on the Hugo mailing list if this Hugo module config makes sense -- i.e., separating out a project's dependencies?

Yes, I asked on the hugo discourse list, but I that didn't help me much. I agree that it's a bit akward to separate out dependencies into a separate module. But I really think is quite clean and I like it much more than the --config or --configDir route. I would like to keep and to go with this approach.

  1. My guess is that, some time in the future, if we decide that we no longer need to support docsy use via git submodules, then we'll be able to eliminate the dependencies module so that users can specify only theme = ["github.com/google/docsy"], is that correct?

Yes, that's correct. Then we can merge both modules into one and we are set.

  1. While this work in any way be impacted by Use NPM packages for Bootstrap and Font Awesome #870 if/when it gets resolved? (/cc @geriom)

As long as we don't change the directory structure inside assets/vendor, I don't expect problems.
But that certainly depends on how #870 will be implemented. Once #870 will be filled with implementation details, I will have a look and come up with a more informed answer to your question.

  1. What are your thoughts on the relative ease (or not) of keeping Bootstrap and Fontawesome versions in sync across supported methods of using docsy (and the explicit assets imports in the files)? I'm asking because this PR introduces an extra set of dependencies to keep in sync with the others. (Maybe Use NPM packages for Bootstrap and Font Awesome #870 will make it a bit easier.)

Frankly spoken, I was a bit surprised to see that NPM packages will be implemented to replace git submodules. As pointed out by you, we then have several competing approaches and keeping them in sync may be a challenge indeed. I'm not against npm at all, it's popular for a good reason. But for me, hugo modules are the way to go, that's what hugo team is pushing forward. And docsy is a hugo theme, so it might be a good decision to follow this given path. As pointed out earlier, using the _vendor directory, we can come up with a go-less (and if wanted even a go- and git-less installation). Then installation is as easy as:

wget https://github.com/google/docsy/archive/refs/tags/v0.1.0-go-git-less.zip
unzip v0.1.0-go-git-less.zip
cd docsy
hugo serve

or

git clone -b skeleton https://github.com/google/docsy/quickstarter.git
cd docsy
hugo serve

No (extra) npm installation needed in this case. That's my vision, and time permitting, I will push this forward.

@chalin
Copy link
Collaborator

chalin commented Feb 3, 2022

Thanks for answering my questions @deining. I'm looking forward to the updates to this PR, after which I believe that it'll be good to go.

As for dropping git submodules for BS & FA in favor of NPM packages, see my comments in #870 (comment).

@deining
Copy link
Collaborator Author

deining commented Feb 6, 2022

  1. Is it correct to say that the Hugo module github.com/google/docsy/module contains docsy's dependencies, and only the dependencies (and the file workaround -- I like how we can localize the workaround)?

Yes.

If so, I think that a better name could be github.com/google/docsy/dependencies don't you think? Regardless, I think that to have a module named module doesn't feel right to me :) ... but that's a small detail.

I agree that github.com/google/docsy/module wasn't the best choice. Let me reflect on this a bit.

Changed to github.com/google/docsy/dependencies in e444964, as requested.

@deining deining requested review from chalin and LisaFC February 6, 2022 16:18
Copy link
Collaborator

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @deining, we're almost there! See inline questions and comments.

As I mentioned earlier, I'd rather that there be no changes to the userguide directory. In particular, why is there a go.mod file specifying that the userguide is a part of the github.com/google/docsy module? (As for the comment in the config file, I can understand why we might want to keep it there.)

config.toml Outdated Show resolved Hide resolved
userguide/go.mod Outdated Show resolved Hide resolved
userguide/config.toml Outdated Show resolved Hide resolved
userguide/config.toml Outdated Show resolved Hide resolved
@deining
Copy link
Collaborator Author

deining commented Feb 11, 2022

Thanks for the updates @deining, we're almost there! See inline questions and comments.

As I mentioned earlier, I'd rather that there be no changes to the userguide directory.

I thought about this and come up with a different solution that I would like to propose:

Why don't we turn the userguide into a hugo module the go-less way? I implemented this in a new branch userguide-module-goless, ahead of branch feature-hugo-module by one single commit. With this branch/commit in place, a preview of the userguide is now as easy as

git clone -b userguide-module-goless https://github.com/google/docsy
cd docsy
hugo serve

No --themesDir= ../.. needed, no goor npm needed, couldn't be any easier. This way we are following the new module route that hugo maintainers dedicate themselves to and we do have a (more or less) seamless transition for existing users.
Apart from that, this approach has another advantage: we now have the directory userguide/_vendor inside our repo, and we can easily reuse it by simply copying it over to the site root of new, go-less installations of any kind (moght be skeleton, based on example site or even on user guide). If we agree to take that approach over, I will go through the module user docs (#802) again and will check how well this works out. If it works out, I will add an additional chapter explaining how to setup docsy using hugo modules in a go-less way (thought about this for a while already).
In general, I like the idea of having the _vendor directory inside the repo, I think this is superior to distributing the _vendor directory as .zip or whatever archive with each new release.

In particular, why is there a go.mod file specifying that the userguide is a part of the github.com/google/docsy module? (As for the comment in the config file, I can understand why we might want to keep it there.)

If we stick to this proposed module route, I think it's natural to mark the userguide itself as module. Apart from that, if we go this route, I would prefer to use the modern [module] notation inside userguide/config.toml rather than sticking to the traditional theme = "..." approach.

@LisaFC
Copy link
Collaborator

LisaFC commented Feb 11, 2022

I think for now I'd prefer to leave the userguide as-is - its current setup works, it works with our continuous deployment/previews from Netlify, and I think the likelihood of anyone wanting to just pull down the user guide to edit/serve minus the rest of the theme is pretty small (generally community members who edit the docs do so along with a theme update). What are the advantages of making it into a module, other than "it is the newest Hugo way"?

We can then look at refactoring it as a module itself once we've sorted the general "use Docsy as a Hugo module".

@deining
Copy link
Collaborator Author

deining commented Feb 11, 2022

I think for now I'd prefer to leave the userguide as-is - its current setup works, it works with our continuous deployment/previews from Netlify,

I deployed the user guide on Netlify the hugo module way, and that worked out of the box, too.

and I think the likelihood of anyone wanting to just pull down the user guide to edit/serve minus the rest of the theme is pretty small (generally community members who edit the docs do so along with a theme update).

That may be true for the moment, but we should also seek perspectives for the future. One thing that I have in mind: Novice users could easily take the directory userguide, copy it to their projects folder, rename it and take it as starting point for thier new site (like they can do with the docsy-example site already now). Advantage of the userguide (go-less, hugo module): it is fully self contained (see below).

My hope: once novice users see how easy it is to invoke the user guide via the module way, they are more inclined to go this path for their own new installation. I think we can accelerate the transition to the use of docsy as hugo module this way, and that's a good thing IMHO.

What are the advantages of making it into a module, other than "it is the newest Hugo way"?

  • It's more than 20 times faster (36 s vs 1.7 s)
  • Download size is more than 10 times smaller (268 MB vs. 22 M).
  • Command line call is easier to remember (hugo serve --themesDir ../.. vs. simply hugo serve)
  • As mentioned above: the go-less directory userguide is fully self-contained, thus it can serve as an ideal starting point for your own site. No checkout of theme and/or submodules required, simply copy and rename the directory, enter it and run hugo serve. Couldn't be any easier IMHO, and lowering the barrier for novice users should be a major concern.

You are invited to check these perfomance figures noted above by yourself:

Traditional:

$ time git clone --recurse-submodules --depth 1 https://github.com/google/docsy.git && cd docsy/userguide/ && hugo server --themesDir ../..  &
real    0m36.636s
du -sh ..
268 M

Hugo module

time git clone -b userguide-module-goless https://github.com/google/docsy && cd docsy/userguide/ && hugo serve &
real    0m1.679s
du -sh ..
22 M

@chalin
Copy link
Collaborator

chalin commented Feb 14, 2022

I think for now I'd prefer to leave the userguide as-is ... We can then look at refactoring it as a module itself once we've sorted the general "use Docsy as a Hugo module".

This is my preference as well: let's get Docsy-as-a-hugo-module in first.

Copy link
Collaborator

@chalin chalin left a comment

Choose a reason for hiding this comment

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

@deining: the PSC was in agreement that we'd like to bring in the docsy-as-hugo-module feature without any changes to the userguide -- please file a separate issue for the config file refactoring. Can you make the remaining changes that were requested for this PR?

@deining
Copy link
Collaborator Author

deining commented Feb 14, 2022

@deining: the PSC was in agreement that we'd like to bring in the docsy-as-hugo-module feature without any changes to the userguide -- please file a separate issue for the config rile refactoring. Can you make the remaining changes that were requested for this PR?

Just commited the requested changes into branch feature-hugo-module. Thanks for re-reviewing my PR.

Copy link
Collaborator

@chalin chalin left a comment

Choose a reason for hiding this comment

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

LGTM after you rebase (I've tried to rebase but I can't force push). Thanks @deining!

@deining
Copy link
Collaborator Author

deining commented Feb 18, 2022

Rebased the branch as requested.
Please note that head of branch has two tags v0.2.0-pre and dependencies/v0.2.0-pre, I'm refering to this tags in the the extended docs (#802). I appended -pre since documentation is still in review and needs most likely some refinement.

@chalin chalin merged commit 1e5f31c into master Feb 18, 2022
@chalin
Copy link
Collaborator

chalin commented Feb 18, 2022

@LisaFC - this has been merged, so (re-)work of the docs can being :), thanks!
@deining - great work! It's finally merged! 🎉

@LisaFC
Copy link
Collaborator

LisaFC commented Feb 28, 2022

Fantastic - I have some time this week and will revise the docs!!

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

Successfully merging this pull request may close these issues.

3 participants