Skip to content

feat: specify add-on options in command args #543

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

AdrianGonz97
Copy link
Member

@AdrianGonz97 AdrianGonz97 commented Apr 22, 2025

partially implements #528 (although i'm moreso playing around with this idea atm)

While similar to #528, there's a minor difference when it comes to bailing/defaults:

# prompt for tailwind options
sv add tailwind

# installs tailwind with only the `forms` plugin
sv add tailwind=plugins:forms

# Rather than bailing on incomplete options, we'll apply defaults to incomplete items (just like we do today):
# installs tailwind with default options (which means no plugins are included)
sv add tailwind=

# installs drizzle with default options (postgres.js and no docker)
sv add drizzle=database:postgresql

I also had to configure our own formatHelp method to add a new help field that nicely displays the add-on options:

➜  boo sv add -h     
Usage: sv add [options] [add-on...]

applies specified add-ons into a project

Arguments:
  add-on                            add-ons to install

Add-On Options:
  tailwindcss                       plugins: typography, forms
  sveltekit-adapter                 adapter: auto, node, static, vercel, cloudflare-pages, netlify
  drizzle                           database: postgresql, mysql, sqlite
                                    client: postgres.js, neon, mysql2, planetscale, better-sqlite3, libsql, turso
                                    docker: yes, no
  lucia                             demo: yes, no
  paraglide                         languageTags: <user-input>
                                    demo: yes, no

Options:
  -C, --cwd <path>                  path to working directory (default: "...")
  --no-preconditions                skip validating preconditions
  --[no-]install <package-manager>  installs dependencies with a specified package manager (choices: npm, yarn, pnpm, bun, deno)

It would also be nice to have an example on how to apply them (e.g. Usage: sv add drizzle=database:postgresql,client:postgres.js,docker:yes), but I'm not exactly sure where that should go in the help text just yet.

Issues

Note

This has been resolved by requiring options to be listed with the <name>:<value> format (e.g. sv add addon=option:value)

I didn't notice this before, but the paraglide add-on is a bit of a nuisance when it comes to specifying its options. This is because one of its options prompts for arbitrary user input.

For example, how do we parse this?

sv add paraglide=en,es,de,no-demo
# ideally, we'll want to yank out `en,es,de` and `no-demo`

Since add-on options are delimited by ,'s and paraglide uses a , to delimit its own user input, how should we parse this? I'm not exactly sure yet, but this is an issue that occurs to this day with our current add-on option flags format: --paraglide=en,de,demo. I guess one option would be to choose a new delimiter for the paraglide add-on. Alternatively, we could change it to a ; for the add-on args (e.g. sv add paraglide=en,de;no-demo).

Copy link

changeset-bot bot commented Apr 22, 2025

⚠️ No Changeset found

Latest commit: 37e6f34

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

pkg-pr-new bot commented Apr 22, 2025

Open in StackBlitz

npx https://pkg.pr.new/sveltejs/cli/sv@543
npx https://pkg.pr.new/sveltejs/cli/svelte-migrate@543

commit: 37e6f34

@AdrianGonz97 AdrianGonz97 marked this pull request as draft April 22, 2025 21:49
@benmccann
Copy link
Member

Ah, interesting catch on the paraglide add-on. The only other option I can think of is to do what CSV does when there's a comma and wrap that value in quotes:

sv add paraglide="en,es,de",no-demo

@AdrianGonz97
Copy link
Member Author

That's an interesting idea that could work!

Another idea that might work would be to eliminate the ambiguity of specifying options which would act as it's own separator as well:

sv add paraglide=languageTags:en,es,de,demo:no

sv add drizzle=database:postgres,client:postgres.js,docker:yes

However, it's a bit more verbose and can get awkward around the y/n prompts (e.g. docker:no, demo:yes, etc.)

@benmccann
Copy link
Member

Adding the option name in there does make things clearer. Though it's still a little hard to read since , can be used in multiple ways and still might not be a bad idea to switch to ; anyway:

sv add paraglide=languageTags:en,es,de;demo:no

sv add drizzle=database:postgres;client:postgres.js;docker:yes

@AdrianGonz97
Copy link
Member Author

AdrianGonz97 commented Apr 23, 2025

Sadly the quotes method (sv add paraglide="en,es",no-demo) won't work as shells strip the quotes from the args before we're able to parse them (e.g. it gets converted into sv add paraglide=en,es,no-demo).

You can view this in action by running node --eval "console.log(process.argv)" foo="bar" locally.

Though it's still a little hard to read since , can be used in multiple ways and still might not be a bad idea to switch to ; anyway:

Good point! Using a ; too will also make it notably easier to parse as well. Will give it a go


edit: Using a ; won't work either as it signifies the end of a command and the start of a new command in shells 😅 ironically, to get it to work with semis, we'd have to wrap the whole thing in quotes: sv add paraglide="languageTags:en,de;demo:yes"

@AdrianGonz97
Copy link
Member Author

I've updated the PR to now use the addon=<name>:<value> format for specifying options. This solves the paraglide issue and seems to be working nicely

@manuel3108
Copy link
Member

Generally, this seems like the perfect solution. We should have found that way earlier. It's way more descriptive and more clear what you are actually trying to achieve. Also the first tests seemed pretty successful.

But this currently fails for me

pnpm dlx https://pkg.pr.new/sveltejs/cli/sv@543 add drizzle=database:postgresql,client:postgres.js --no-install

Probably because it does not know if the comma should separate an option or an option value (like for tailwind forms). Alternatively, this might be on of those things that's only happening on windows.

The same goes for this one:

pnpm dlx https://pkg.pr.new/sveltejs/cli/sv@543 add paraglide=languageTags:en,de,demo:no --no-install

Alternatively, I'm just not aware of the correct syntax.

It would also be nice to have an example on how to apply them (e.g. Usage: sv add drizzle=database:postgresql,client:postgres.js,docker:yes), but I'm not exactly sure where that should go in the help text just yet.

I wouldn't necessarily deem this as obligatory. Based on the docs Ben drafted, this should be pretty clear how to use them.

@AdrianGonz97
Copy link
Member Author

AdrianGonz97 commented Apr 24, 2025

But this currently fails for me

pnpm dlx https://pkg.pr.new/sveltejs/cli/sv@543 add drizzle=database:postgresql,client:postgres.js --no-instal

Running this line works perfectly for me on linux, so I'm going to guess it's Windows 🙃. Will take a closer look in a few.

@manuel3108
Copy link
Member

manuel3108 commented Apr 24, 2025

Here is a stack trace if you want:

■  Error: Unable to process 'src/lib/server/db/schema.ts'. Reason: unreachable state...
│      at Object.file (file:///C:/Users/manue/AppData/Local/pnpm-cache/dlx/549e25a5f22fa51d3cd0c210a4923d430768b3b4648911fef330d82ee1b55e7b/19668831d42-56e70/node_modules/.pnpm/sv@https+++pkg.pr.new+sveltejs+cli+sv@543/node_modules/sv/dist/install-iBxCFLH9.js:1067:35)
│      at Object.run (file:///C:/Users/manue/AppData/Local/pnpm-cache/dlx/549e25a5f22fa51d3cd0c210a4923d430768b3b4648911fef330d82ee1b55e7b/19668831d42-56e70/node_modules/.pnpm/sv@https+++pkg.pr.new+sveltejs+cli+sv@543/node_modules/sv/dist/bin.js:1299:6)
│      at runAddon (file:///C:/Users/manue/AppData/Local/pnpm-cache/dlx/549e25a5f22fa51d3cd0c210a4923d430768b3b4648911fef330d82ee1b55e7b/19668831d42-56e70/node_modules/.pnpm/sv@https+++pkg.pr.new+sveltejs+cli+sv@543/node_modules/sv/dist/install-iBxCFLH9.js:1108:14)
│      at applyAddons (file:///C:/Users/manue/AppData/Local/pnpm-cache/dlx/549e25a5f22fa51d3cd0c210a4923d430768b3b4648911fef330d82ee1b55e7b/19668831d42-56e70/node_modules/.pnpm/sv@https+++pkg.pr.new+sveltejs+cli+sv@543/node_modules/sv/dist/install-iBxCFLH9.js:1023:50)
│      at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
│      at async runAddCommand (file:///C:/Users/manue/AppData/Local/pnpm-cache/dlx/549e25a5f22fa51d3cd0c210a4923d430768b3b4648911fef330d82ee1b55e7b/19668831d42-56e70/node_modules/.pnpm/sv@https+++pkg.pr.new+sveltejs+cli+sv@543/node_modules/sv/dist/bin.js:7071:79)
│      at async file:///C:/Users/manue/AppData/Local/pnpm-cache/dlx/549e25a5f22fa51d3cd0c210a4923d430768b3b4648911fef330d82ee1b55e7b/19668831d42-56e70/node_modules/.pnpm/sv@https+++pkg.pr.new+sveltejs+cli+sv@543/node_modules/sv/dist/bin.js:6837:25
│      at async runCommand (file:///C:/Users/manue/AppData/Local/pnpm-cache/dlx/549e25a5f22fa51d3cd0c210a4923d430768b3b4648911fef330d82ee1b55e7b/19668831d42-56e70/node_modules/.pnpm/sv@https+++pkg.pr.new+sveltejs+cli+sv@543/node_modules/sv/dist/bin.js:3899:3)

The second paraglide option actually installs the demo, even though it's explicitly set to no. But this one does not throw an error

This was referenced Apr 24, 2025
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