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

feat: add create and add commands #16

Merged
merged 51 commits into from
Sep 5, 2024
Merged

Conversation

AdrianGonz97
Copy link
Member

@AdrianGonz97 AdrianGonz97 commented Aug 30, 2024

this PR is a chunky one that contains several necessary architectural changes to core due to how interwoven it was to the previous cli

Changes

  • added commands for create and add
    • sv create [path]
    • sv add [adder...]
  • renamed the default template (which is the sverdle demo template) to demo
  • reworks core such that only the bare essentials are left:
    • all cli related work is now done in cli (cli parsing, prompting, etc), while any adder specific functionality, such as definitions, file processing and tooling, remain in core
    • remove unused features like postconditions, installHooks, and uninstallHooks.
    • tried to reorganize the package to make logical sense (still working on this)
  • for the cli:
    • got rid of as much indirection as i could. everything now reads more plainly and flat
    • also opted to remove the prompts wrapper since it didn't provide much value besides being another layer of abstraction
  • testing should now be much simpler now that we have dedicated APIs for creating projects and installing adders (create and installAdders)
  • moved config directly into adders since they are so closely related and to better ensure that imports and categories are updated simultaneously when new adders are created

TODOs before merging

  • finish setting up community adders
  • settle on how adder options should be specified as CLI args
    we've settled on using the following format for now until we find a nicer syntax down the line:
# adders can be specified simply via args, which will result in the user being prompted
# for adder questions
sv add drizzle tailwindcss
# adder option flags could be used instead to specify option values
sv add --drizzle=postgresql,postgres.js,no-docker --tailwindcss=no-typography
# a combination of both should will work as well
sv add tailwindcss --drizzle=postgresql,postgres.js,no-docker
  • setup custom help command to display adder choices and options
    here's how the choices will be printed:
Usage: sv add [options] [adder...]

Applies specified adders into a project

Arguments:
  adder                       adders to install

Options:
  -C, --cwd <path>            path to working directory
  --no-install                skips installing dependencies
  --no-preconditions          skips validating preconditions
  --default                   applies default adder options for unspecified options (default: false)
  --community <adder...>      community adders to install (default: [])
  --tailwindcss <options...>  (choices: typography, no-typography)
  --drizzle <options...>      (choices: postgresql, mysql, sqlite, postgres.js, neon, mysql2, planetscale, better-sqlite3, libsql, turso, docker, no-docker)
  -h, --help                  display help for command

here are a few adder flag options that we've discussed:

# adders are specified with `--addername` flags with their options provided as a list.
# this syntax is concise, but would force us to remove specifying adders through plain args:
# e.g. `sv add drizzle tailwindcss` for the simple and most common case would not be allowed.
# instead, they'd have to do this: `sv add --drizzle --tailwindcss`, which isn't as nice
sv add --drizzle=postgresql,postgres.js,no-docker --tailwindcss=no-typography

# not the cleanest, and is arguable redudant in name, but it's intention is clear when reading
sv add drizzle tailwindcss --drizzle-options=postgresql,postgres.js,no-docker --tailwindcss-options=no-typography

# this one's a bit whacky, but a single --options flag with a space-separated list 
# of options that correspond to the args order. in the below example, 
# options `postgresql,postgres.js,no-docker` would be applied to `drizzle`.
# it's relatively concise, but it may not be as intuitive and is arguably harder to mentally parse
sv add drizzle tailwindcss --options postgresql,postgres.js,no-docker no-typography

# as concise as the first option, but I don't think this is a common/semantically correct syntax,
# so someone who's experienced with CLIs may get confused
sv add drizzle=postgresql,postgres.js,no-docker tailwindcss=no-typography

# we could have an `advanced` mode where only flags can be used to specify which adders to install
sv adv add --drizzle=postgresql,postgres.js,no-docker --tailwindcss=no-typography
# and the default mode would be the `simple` mode, not taking any adder flags:
sv add drizzle tailwindcss

Copy link

pkg-pr-new bot commented Aug 30, 2024

Open in Stackblitz

pnpm add https://pkg.pr.new/sveltejs/cli/@svelte-cli/ast-manipulation@16
pnpm add https://pkg.pr.new/sveltejs/cli/@svelte-cli/ast-tooling@16
pnpm add https://pkg.pr.new/sveltejs/cli/sv@16
pnpm add https://pkg.pr.new/sveltejs/cli/@svelte-cli/core@16

commit: e3950c5

@manuel3108
Copy link
Member

If I already provided a path to the create cli like this, then it should skip the question "Where should we create your project". With that change the create command could be run without any interactions, if all other arguments are created.

this is how I originally had it (line 47) and I've gone back and forth on it. I ended up liking having some kind of confirmation prompt that ensures that we're creating in the correct directory. i could still be swayed into either camp though

I'm still not convinced this is necessary. We already have the prompt that will ask you to override files if we detect some files (as a last resort). Accidentally creating a project in a wrong directory after specifying a path, shouldn't be a problem we should care about in my opinion.

Not sure about the naming of the cwd option. If you provide a path, then its no longer your current working directory. I think path as we had it previously should work better.

I find --path to be a bit too generic and that --cwd makes the most sense given that you're changing the working directory of the CLI. often times, i find this option presented in CLIs as a -C flag too (git, pnpm, nuxi, etc)

valid point. Another option would --directory or --dir, but i'm fine with everything.


As for the advanced mode, I see your point. I was thinking like this: If we have sv advanced add what would sv advanced create do. And i did not come to any conclusion. But after re-thinking it, I think what you proposed is more explicit. And since this will probably only be used in ci, I don't think we should bother too much.

Also a call to suggestInstallingDependencies after the create command would make sense i think, even if this is currently not the case for create-svelte.

this should already be the case. is it not prompting you after creation?

Nope, it's not. Even if i'm outside of the mono-repo:

Output

PS D:\dev\web\svelte-cli> node .\packages\cli\dist\ create ../svelte-cli-temp --no-adders
┌  Welcome to the Svelte CLI! (v0.5.0)
│
◇  Where should the project be created?
│  ..\svelte-cli-temp
│
◇  Which Svelte app template
│  Skeleton project
│
◇  Add type checking with Typescript?
│  Yes, using Typescript syntax
│
◇  Project created
│
└  You're all set!

Aside from that I think renaming our core package to add would make a lot of sense. Moving the adders inside the add package would be my next logical thing.

hmmm, i'm not so sure about that yet. that would mean shipping the official adders as part of the core package as well

As far as i remember, the plan was to only really publish sv and some of our other dependencies like ast-tooling. I thought that everything else would be bundled in, that's why I made this comment.

@AdrianGonz97 AdrianGonz97 changed the title feat(DRAFT): add create and add commands feat: add create and add commands Sep 4, 2024
Copy link
Member

@manuel3108 manuel3108 left a comment

Choose a reason for hiding this comment

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

LGTM, there is just one minor bug i think.

pnpx https://pkg.pr.new/sveltejs/cli/sv@16 add --drizzle=postgresql,neon

Based on the PR description this should work, but I'm getting this output:

Invalid '--drizzle' option: 'postgresql neon'
Available options: postgresql, mysql, sqlite, postgres.js, neon, mysql2, planetscale, better-sqlite3, libsql, turso, docker, no-docker

Further we might be able to clarify the help screen:

  --drizzle <options...>      (choices: postgresql, mysql, sqlite, postgres.js, neon, mysql2, planetscale, better-sqlite3, libsql, turso, docker,        
                              no-docker)

I would suggest something like this (rough idea):

  --drizzle <options...>      (choices: 
                              database: postgresql, mysql, sqlite,
                              provider:  postgres.js, neon, mysql2, planetscale, better-sqlite3, libsql, turso,
                              docker: docker,  no-docker)

This will make it more clear which options can / should be combined together. Users might try adding postgresql,mysql,sqlite at the same time otherwise.


From this point on, feel free to merge anytime.

@AdrianGonz97
Copy link
Member Author

Further we might be able to clarify the help screen:

  --drizzle <options...>      (choices: postgresql, mysql, sqlite, postgres.js, neon, mysql2, planetscale, better-sqlite3, libsql, turso, docker,        
                              no-docker)

I would suggest something like this (rough idea):

  --drizzle <options...>      (choices: 
                              database: postgresql, mysql, sqlite,
                              provider:  postgres.js, neon, mysql2, planetscale, better-sqlite3, libsql, turso,
                              docker: docker,  no-docker)

This will make it more clear which options can / should be combined together. Users might try adding postgresql,mysql,sqlite at the same time otherwise.

something like this would be ideal, yeah. however it would require rethinking how the drizzle adder works with it's options given that the options are not sorted in this way. in its current state, we would have to apply special logic just for drizzle to achieve this

@manuel3108
Copy link
Member

You are right, did not think about that. Let's leave it like this for now, we can always explore this at a later stage, when we have merged all other relevant adders (lucia, supabase, etc)

@AdrianGonz97 AdrianGonz97 merged commit 34c51f5 into main Sep 5, 2024
4 checks passed
@AdrianGonz97 AdrianGonz97 deleted the feat/add-subcommands branch September 5, 2024 15:57
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

Successfully merging this pull request may close these issues.

3 participants