-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat: create-svelte
support for scoped packages
#4851
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
Conversation
🦋 Changeset detectedLatest commit: 036752d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
create-svelte
support for scoped packages
const regex = new RegExp(/^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$/); | ||
const match = regex.test(value); | ||
return !match | ||
? 'Value is not a valid NPM package name. Package names must be lowercase and one word, and may contain hyphens and underscores, or be scoped and of the form @scope/package.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't match
already boolean value? Also I don't know if You want to put error message here.
return match ?? 'Value is not...'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you're sort of right. The error message would never be returned with ??
(nullish coalescing) -- but I should be using matched || "Blah blah blah"
. I'll fix it later.
Some thoughts: If you use create-svelte to initialize an application, asking for a package name isn't really relevant and could be confusing. In fact if the user doesn't want to publish a package, it would be best if If you use it to initialize a library, esp. within an org, it is very likely that the org directory already exists on disk and you run If we end up asking for a package name and validating it, this one is the official utility to do so: https://github.com/npm/validate-npm-package-name |
name: 'packagename', | ||
message: 'What would you like to name your project?', | ||
validate: (value) => { | ||
const regex = new RegExp(/^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
~ is no longer allowed for new packages, see https://github.com/npm/validate-npm-package-name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting! That's probably a second bug fix for us -- the current function we're using to convert folder names to package names is still allowing them. I'll likely change this implementation to use the library you linked in your top level comment anyway -- I didn't realize that was an official NPM library when I saw it!
I'd agree... to an extent? IMO, if you're creating a Node application, the package name is always relevant, but others may disagree. (More below.)
I'm not really sure what mishaps we'd be preventing (oops, I accidentally went all the way through the multistep process of releasing a package when it was supposed to be private? 😂). Maybe I'm just not thinking of something obvious.
Yeah, I supposed what the default is depends on what people are normally doing. I normally run
Thank you! I'm going to incorporate this into this PR. Just to add to the discussion and source your feedback (and @Mlocik97's as he seems pretty invested in this one): I cannot think of a better way to support scoped packages. No matter what, it seems like there's no way to get around asking for a package name. Deriving a scoped package from the file system seems error-prone and confusing. You could add a second prompt after asking for the location ("Would you like the current package name?: ${package_name_derived_from_folder}"), but that still involves a second prompt that, unlike the current implementation, can't be skipped by providing an argument to the CLI. The only other palatable option I could think of is providing an override package name to the CLI as the second positional argument ( If we don't want to support scoped package names at all, we can just close this PR and the associated issue. 🙂 |
I really don't know what is bad about this... how
How, I already wrote how it should work (in original issue), and I don't think it's bug-prone or confusing, actually opposit. |
What behavior I expect is: 1 will generate app inside that folder and set pkg.name to
will generate app inside that folder and set pkg.name to
will generate app inside
will generate app inside
will generate app inside and that all without additional input needed (if user don't want to change it) |
actually I got another idea. What about having it all as it is now... aka exactly how const scope = process.argv[3];
pkg.name = scope
? `@${to_valid_package_name(scope)/to_valid_package_name(name)}`
: to_valid_package_name(name); this would works perfectly I think, would actually do everything we want... Easy to implement, derivation work, and it's simple for both projects without scope and with one (for packaging). @tcc-sejohnson @dominikg |
I had a similar idea that I like slightly better. Right now, the CLI looks like: npm init svelte {path} We just update it to look like (similar but not identical to your suggestion): npm init svelte {path} {packageNameOverride} For calls that specify path but not a package name override, we'll parse the package name from the path as we currently do (with an update that checks to see if the second folder from the leaf starts with an @ - if it does, it'll parse it as a scoped package). For calls that specify a path and a package name, it'll use both verbatim (obviously validating the package name argument). Give me some time today to implement. I think it's the cleanest solution -- the CLI will still work exactly as it currently does, but with the option to specify the package name separate from the path on the CLI. |
Please do not put work into this before it was discussed and decided if more alternatives
|
dominkg if it would be introducing another param, I don't think it's problem. without that param whole create-svelte would works exactly as it does now. So I don't see any breaking change or confusion... we just introduce another param for scope or pkg.name overwrite. I don't see how this would be problematic. tcc-sejohnson I don't think dominikg said this PR is wrong, more than he said they need time to discuss it with team to decide how it will work. |
I understand! I still have the work saved. I would rather not clog up the PR list (it's already pretty long). Whenever the maintainers join the discussion on the linked issue, if they decide to support a change, I'll open a new PR. They seem pretty overwhelmed (not in a bad way!) with the number of issues/PRs open in the 1.0 milestone right now, so I didn't want to have something requiring additional discussion open. If/when they tag more "help wanted" issues (or when I find a way to get in on conversations involving non-help-wanted issues), I'll jump on them. |
Fixes #4754.
Currently
create-svelte
does not support creating projects with scoped names, i.e.@my-fancy-scope/my-fancy-package
. This is a crack at fixing that in a way that's as un-frustrating to developers as possible.The current approach is to try to turn the base path of the provided project location into a package name. This has some weird edge cases, and is especially gross with
@scope/package
semantics, as that's obviously two folders (or a folder and a file with no extension) from a filesystem perspective.For ease of understanding, here's the current behavior vs. the new behavior:
./@myscope/mypackage
becomesmypackage
, and./.my folder name with spaces
becomesmy-folder-name-with-spaces
).Where should we create your project? (leave blank to use default: ${provided_package_name})
@my-scope/my-package => ./@my-scope/my-package
my-package => my-package
All in all, this adds one prompt and makes the project naming much more user-controllable. At worst, it requires typing out one more answer than current -- at best, it's still the same number of inputs.
One other fringe benefit of this is that the
to_valid_package_name
(and the scoped package edge-cases that come with it) function is no longer needed at all -- since the package name is validated at the CLI, we know it's a valid package name already.Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpx changeset
and following the prompts. All changesets should bepatch
until SvelteKit 1.0