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

Refactor CLI to use "commands" #33

Closed
JordanShurmer opened this issue Jan 28, 2021 · 13 comments
Closed

Refactor CLI to use "commands" #33

JordanShurmer opened this issue Jan 28, 2021 · 13 comments

Comments

@JordanShurmer
Copy link
Contributor

First, this project is great. Keep up the good work.

This is a proposal for a redesign of the CLI interface. I'd be happy to put in a PR for if it seems like a good thing to do.

A Proposal

Redesign the CLI interface to use "commands" instead of options.

$ lume --help

  🔥lume v0.14.0
  A static site generator for Deno

  Docs:

    https://lumeland.github.io

  Usage:

    lume [OPTIONS] COMMAND

  Options:

    -h, --help     - Show this help.
    -V, --version  - Show the version number for this program.

  Commands:

    build [config]  - build the site
    init            - create a _config.js file for a new site
    serve           - Build the site and start a web server that refreshes automatically for every change
    upgrade         - Upgrade local lume install to the latest version
    update          - Update the lume version imported in _config.js to the latest

Each command would have its own --help output as well, specifying the relevant options for that command


If this looks like a good direction for lume I can get a PR ready sometime this week

@shadowtime2000
Copy link
Contributor

I really like this idea! 11ty is a lot simpler than Lume, and Lume is a lot more pluggable and flexible and configurable so I think it is fit that we adopt a more "powerful" CLI system. I think we could use yargs for this.

@probins
Copy link
Contributor

probins commented Jan 28, 2021

I agree this would be better but, f you're going to do this, I think lume COMMAND [OPTIONS] would be more logical. The options only apply to some commands, and the help should make this clear. There is also the run command, and dev, though I'm not sure what effect this has.

I prefer -v to -V (and I wish there was consistency in this across all scripts/systems :-( ). I also think it would be useful if this command (or is it an option?) let the user know if a later version is available. Creating a separate update command raises another issue. The configuration docs use import lume from "https://deno.land/x/lume/mod.js";, i.e. always use the latest version. This means that if you have installed lume and you run it when a newer version is available, it uses the new version, but the installed script is not updated. So, -v gives you a different version from what is actually used. This needs redesigning, I think.

And one more thing: I think build should remain the default command.

@oscarotero
Copy link
Member

Hi.
Thank you very much to bring this up. Casually, I have been working on a similar change a couple of days ago, in a different branch (https://github.com/lumeland/lume/blob/feature/cli-refactor/cli/help.js). I like the idea of having a --help option for every command, I don't know if yargs can do that automatically.

I also agree that build should be the default command, so running lume or lume build are equivalent. To me, --serve is an option of build.

And about the update, this is an undocument command that I'm testing because Deno caches the modules forever, so no matter if you install the latest version of lume, if your _config.js file imports lume without version, it will use the cached version. Until now, I've fixed it by running deno cache here but this should be used also with all plugins that are not loaded by default (postcss, terser, etc), so it's not the ideal situation. Maybe update is not the most apropiate name, and fix-version or something like that, is better. In addition to that, I found that deno checks always the types before runing:

lume
Check https://deno.land/x/lume/cli.js
Check file:///Users/oscarotero/my-site/_config.js
...

And using fixed versions, this check is done only the first time, then it's cached, so it's a lot faster.

If you want to contribute to the work done in this branch, PR are welcome.
Thanks!

@JordanShurmer
Copy link
Contributor Author

Awesome. Thanks for the quick feedback. Good to see work already being done on this front @oscarotero.

I will check out that branch and contribute where I can.

@probins
Copy link
Contributor

probins commented Jan 29, 2021

if your _config.js file imports lume without version, it will use the cached version

I just retested this and you're right. A couple of weeks ago, I ran lume (the previously upgraded version, no longer the latest) and it fetched the latest version. I thought this was caused by the config file, but it must have been something else. So what's the use case for having both update and upgrade? Both the config file and the cli are importing mod.js, so these should surely always use the same version.

@oscarotero
Copy link
Member

oscarotero commented Jan 29, 2021

@probins Yes, correct. It's a way to ensure that cli.js and mod.js and plugins are using the same version. This is also critical because all these modules share the same cache system. And if they use different versions, they use also different caches.

@probins
Copy link
Contributor

probins commented Jan 29, 2021

an alternative is to have not a config file but a build file. I just created a copy of _config.js with site.build() at the end instead of export default site. Then deno run -A --unstable ./_build.js. Seems to produce the same result. I can set the version in the import of mod.js, and if I want to use a new or other version of lume, I just change the import in the build file.

It's quite common in Node to have a JS API version as well as cli, and this is similar. It removes the need for an installed script, and the problem of updating it.

@oscarotero
Copy link
Member

The cli api has also lume --serve that not only runs a build but also watch changes and update the site accordingly. Or lume --run to run a script registered in the site. So this is why _config.js export the site instance, in order to perform different commands with the same site instance.

Btw, you don't need to copy _config.js, just import it with import site from "./_config.js".

Anyway, why do you think this is an alternative for the caching issue?

@probins
Copy link
Contributor

probins commented Jan 29, 2021

if I no longer use the installed script, I don't have to upgrade it. I specify directly in the build script which version I want to use, so the dependencies should all be loaded from that version. If I also define the same version for the plugin imports I'm using, any cache problems should go away, right?

@oscarotero
Copy link
Member

Yes, correct. In fact, Deno recommends to use always versioned imports (I read it, don't remember where). This is what lume --update does, normalize the imported lume modules in the _config.js file to the same version (it uses the same version of cli but is not required).

Because the problem is not that the installed version (cli.js) is a different version as the imported modules in _config.js, because the cli code only runs the site instance returned by _config.js. The problem is when the _config.js imports several lume modules from different versions (for example: mod.js is v0.8.1 and plugins/terser.js is v0.9.0.

@probins
Copy link
Contributor

probins commented Jan 30, 2021

but that's precisely my point: if both the _config.js and the cli define the version no, and the 2 are updated by 2 separate commands, then there is always the chance they will be out of sync. It's also more work for the user. Perhaps upgrade could always run update.

Anyway, I'm changing all my configs to include site.build(), and will see if I encounter any problems. I'm also using this kind of process in other Deno systems I'm using, as ISTM it's simpler than maintaining installed scripts.

I think this issue can be closed.

@oscarotero
Copy link
Member

Yes, if you don't need any extra functionality that cli provides (like run scripts, watch, server and live-reload features), it's perfectly fine don't install it.

The cli interface is similar to what other node libraries provide in a different package (postcss-cli, webpack-cli, etc). But in Deno, thanks to the ES module, we don't need to create different packages, because if you don't use it, this code won't be downloaded.

Perhaps upgrade could always run update.

Maybe, but update updates the current _config.js file and you can have several projects with lume.

@JordanShurmer
Copy link
Contributor Author

Closed by #34

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

No branches or pull requests

4 participants