Skip to content

Conversation

jycouet
Copy link
Contributor

@jycouet jycouet commented Sep 18, 2025

No description provided.

Copy link

changeset-bot bot commented Sep 18, 2025

🦋 Changeset detected

Latest commit: 3c815a2

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 18, 2025

Open in StackBlitz

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

commit: 5742b16

Copy link
Member

@AdrianGonz97 AdrianGonz97 left a comment

Choose a reason for hiding this comment

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

this looks great! for some reason, i'm unable to push some changes to this pr, so i've added some suggestions below.

Comment on lines +21 to +25
".": "./index.ts",
"./css": "./tooling/css/index.ts",
"./html": "./tooling/html/index.ts",
"./js": "./tooling/js/index.ts",
"./parsers": "./tooling/parsers.ts"
Copy link
Member

@AdrianGonz97 AdrianGonz97 Sep 19, 2025

Choose a reason for hiding this comment

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

we'll have to keep in mind that @sveltejs/cli-core will be a public package in the future, so this will have to be updated back to its original state when community add-ons are released

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will it be @sveltejs/cli-core or maybe under sv like sv/core ?
What about @sveltejs/create & @sveltejs/addons ?

It might be a question for later anyway

Copy link
Member

Choose a reason for hiding this comment

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

Will it be @sveltejs/cli-core or maybe under sv like sv/core ?

its still TBD, but our plans were to have core be its own standalone package so that community add-ons don't have sv as a direct dependency

What about @sveltejs/create & @sveltejs/addons ?

create should probably stay in sv, but there's an argument to be made as to where addons should go. it may be more useful to have it be part of core but we can figure it out when we get there :p

@mrginglymus
Copy link

mrginglymus commented Sep 19, 2025

Thanks, types are coming through now!

There are a few types that could be usefully reexported (TemplateType and LanguageType) to avoid type TemplateType = Parameters<typeof create>[1]["template"];

What's significantly missing though is typed official addons - because they're exported as a list, there's no way have type-safe configured addons:

export const officialAddons = [

Could we change this to

export const officialAddons = {
  prettier,
  eslint,
  ...,
} satisfies Record<string, AddonWithoutExplicitArgs> as const;

or similar? That way a user will be able to define, in a type safe way, a set of preconfigured options for the addons.

@jycouet
Copy link
Contributor Author

jycouet commented Sep 19, 2025

I should not have created the PR! that's why we can't push there. :/ (TIL!)
Now @sxzz invited me to his repo, I'll try to apply suggestions (today) and see 👍

EDIT: it seems that you can push now @AdrianGonz97

@jycouet
Copy link
Contributor Author

jycouet commented Sep 19, 2025

Could we change this to

export const officialAddons = {
  prettier,
  eslint,
  ...,
} satisfies Record<string, AddonWithoutExplicitArgs> as const;

or similar? That way a user will be able to define, in a type safe way, a set of preconfigured options for the addons.

Before exporting this, we need to fix pnpm tsc. (Similar I think)
I'm not sure where it's coming from as the typing was good before (internally). The thing is that they are not all AddonWithoutExplicitArgs.

Who wants to crack the matrix ? :D

@mrginglymus
Copy link

Well I thought I'd fixed pnpm check, but....rolldown doesn't support custom export conditions?

@sxzz
Copy link

sxzz commented Sep 19, 2025

Of course, Rolldown supports it.

@mrginglymus
Copy link

mrginglymus commented Sep 19, 2025

Something in my current toolchain (via rollup-plugin-dts) is dropping it...

@manuel3108 manuel3108 marked this pull request as draft September 24, 2025 16:12
@manuel3108
Copy link
Member

Converted to draft, dosn't seem ready yet according to the comments

@AdrianGonz97
Copy link
Member

AdrianGonz97 commented Sep 25, 2025

exposing the officialAddons map is turning into a bit of a head scratcher with relation to tsdown.

atm, i'm trying to disable oxc (as well as disabling isolatedDeclarations) and use tsc instead to generate the types through tsdown, but it seems that rolldown-plugin-dts is getting hung up on some of the types that originate from @sveltejs/create, and are reexported from the cli:

[plugin rolldown-plugin-dts:fake-js]
SyntaxError: Export 'LanguageType' is not defined. (57:35)

i have a feeling this may be a rolldown-plugin-dts bug, but im unsure. have any ideas @sxzz?

@AdrianGonz97
Copy link
Member

i guess we could keep using isolatedDeclarations with oxc, but it would force us to generate types that aren't really ideal:

img

with Addon<any>, we lose the types of the addon's options, but keep the typed keys for each entry (e.g. officialAddons.prettier, etc). we'd also have to maintain the OfficialAddons type, but that's a rather small annoyance.

ideally, using as const (shown on the left) would be the best, but it doesn't seem to be compatible with isolatedDeclarations

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.

5 participants