Skip to content

Conversation

jycouet
Copy link
Contributor

@jycouet jycouet commented Sep 2, 2025

Closes #179


It seems that clack team will also go in this direction one day.

Copy link

changeset-bot bot commented Sep 2, 2025

🦋 Changeset detected

Latest commit: bb624c4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
sv Patch

Not sure what this means? Click here to learn what changesets are.

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

Copy link

pkg-pr-new bot commented Sep 2, 2025

Open in StackBlitz

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

commit: bb624c4

@jycouet jycouet marked this pull request as ready for review September 2, 2025 18:43
@benmccann
Copy link
Member

I'm not sure I'm in love with the name prepareAddonOptions. How about addonOptions().option().build() or newAddonOptions().add().build()?

@jycouet
Copy link
Contributor Author

jycouet commented Sep 3, 2025

I'm not sure I'm in love with the name prepareAddonOptions. How about addonOptions().option().build() or newAddonOptions().add().build()?

What about:
defineAddonOptions().add().build() ?

@benmccann
Copy link
Member

Works for me

@43081j
Copy link
Contributor

43081j commented Sep 5, 2025

this works so much better than the object-style definitions before

i think we'll do the same in clack eventually 😅

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.

I.... don't understand Typescript. Why do we need to make it more complicated to make it actually work... (especially referring to the internal createOptionBuilder).

If you ignore the internals though, this is sick. It fixes the issue without changing anything major. Apart from the open comment, I think this should be good to go.

@jycouet
Copy link
Contributor Author

jycouet commented Sep 5, 2025

If you ignore the internals though, this is sick. It fixes the issue without changing anything major. Apart from the open comment, I think this should be good to go.

Spent a few fun hours on this! héhé.
Last comment done now IMO

@manuel3108 manuel3108 merged commit 8876283 into main Sep 5, 2025
8 checks passed
@manuel3108 manuel3108 deleted the tentative-007/typing-option-conditions branch September 5, 2025 14:12
@github-actions github-actions bot mentioned this pull request Sep 1, 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.

Add proper typing for option conditions
4 participants