sveltejs / kit

web development, streamlined
https://kit.svelte.dev
MIT License
18.61k stars 1.92k forks source link

`create-svelte`'s `to_valid_package_name` modifies valid package names #4754

Closed WillsterJohnson closed 1 year ago

WillsterJohnson commented 2 years ago

Describe the bug

According to NPM's docs for package.json;

The "name" field contains your package's name, and must be lowercase and one word, and may contain hyphens and underscores.

Which if taken in a vacuum, the to_valid_package_name function correctly enforces. What it doesn't correctly enforce is the use of scopes, eg @sveltejs/kit. Instead, it replaces the @ and / with hyphens.

Reproduction

The easiest method is to locally install create-svelte and pass the following when creating the package;

import { create } from "create-svelte";

create("./emptyDir", { name: "@scope/package",
 /* these two don't matter, so long as they're valid */ template: "skeleton", types: "typescript" });

Observe the package.json "name" field doesn't read "@scope/package", bet instead reads "-scope-package"

I traced where that name field goes, and the only place it's modified is this line

Repo with minimum reproduction

Logs

N/A

System Info

System:
    OS: Linux 5.16 Pop!_OS 21.10
    CPU: (8) x64 Intel(R) Core(TM) i5-10210U CPU @ 1.60GHz
    Memory: 9.84 GB / 15.36 GB
    Container: Yes
    Shell: 5.1.8 - /bin/bash
  Binaries:
    Node: 17.6.0 - ~/.nvm/versions/node/v17.6.0/bin/node
    npm: 8.5.1 - ~/.nvm/versions/node/v17.6.0/bin/npm
  Browsers:
    Chrome: 100.0.4896.127
    Chromium: 100.0.4896.127
    Firefox: 99.0

Severity

annoyance

Additional Information

This is working for me, though it's possible these simple lookbehinds miss a rare use case;

/** @param {string} name */
function to_valid_package_name(name) {
  return name
    .trim()
    .toLowerCase()
    .replace(/\s+/g, '-')
    .replace(/^[._]/, '')
-    .replace(/[^a-z0-9~.-]+/g, '-');
+    .replace( /[^a-z0-9~.-@\/]+/g, '-' )
+    .replace( /^(?<=.)@/g, '-' )
+    .replace( /^(?<!@.+)\//g, '-' );
}
Mlocik97 commented 2 years ago

I actually am thinking about what would happen if you would call npm init svelte @something/myapp, I mean, if @something/ define scope, folder name should be just myapp while name in pkg should be set to @something/myapp. Now that I test npm init @something/myapp it seems it's broken too, as it's creating folder @something that contains folder myapp (and it set pkg.name to just myapp (as it's name of nested folder)). But I think this should happen only if there is @ at beginning, in other case, nested folders, and name of the most nested one to pkg.name.

So this should be fixed too...

elliott-with-the-longest-name-on-github commented 2 years ago

I think there are two questions involved here -- behavior on the filesystem and behavior in package.json.

package.json

If and only if the incoming package_name starts with @ and contains one and only one /, the name in package.json is set to package_name with the @ and / preserved, but all other matching rules are applied.

else, the existing rules are applied.

As far as implementation goes, if @willster277's pattern covers all edge cases, yay! If not, we can continue to add more complicated edge-case-catching regexes (🤮), or just do some string tests to figure out which one should be used.

Filesystem

I can think of two possibilities here:

Rich, if you greenlight this, I'm happy to implement it and open a PR.

WillsterJohnson commented 2 years ago

Great responses from both of you, I hadn't considered the nuances of npm init svelte@next @scope/package, I feel like @tcc-sejohnson has covered this quite well, but I'll throw in my two cents on it.

I think for filesystem the first option should be used. It's easier for users if the script works as "we'll put it in the package folder if you don't say otherwise". Having to move files around manually after the project setup tool is run kinda feels like the project setup tool didn't finish setting up the project.

As for import { create } from "create-svelte";, I think the changes to the command line interface won't be any interference so long as the same rules apply. Being able to specify a filepath regardless of the package name is (in my opinion) essential for the module to work correctly. My use case is for creating SvelteKit projects in a monorepo. I want everything in either apps/** or packages/**, and having to move files with node:fs is much more painful than telling create-svelte to put the package in a specific place. (There's also the case of create-svelte maybe having separate templates for package creation and app development, but that's not a topic to discuss here)

elliott-with-the-longest-name-on-github commented 2 years ago

Took a crack at this via sveltejs/kit#4851. We'll see what the maintainers think of it!

Mlocik97 commented 2 years ago

https://github.com/vitejs/vite/issues/8448

Actually how Vite's CLI works with pkg.name could fix this. Just let user write pkg.name via CLI (optionaly), and if they leave input blank, derive it from folder name.

dummdidumm commented 2 years ago

The discussion in sveltejs/kit#4851 once again shows to me that we should have a different flow in create-svelte when creating packages. The first prompt about the project should be enhanced with a third option that is something along the lines of "Svelte package". This would mean that we can

Rich-Harris commented 2 years ago

More generically, perhaps it makes sense for different templates to be able to offer their own prompts in addition to the universal prompts. Like, maybe you could imagine a future where we have many more starter templates, and one of them is a blog, and choosing it prompts you to choose a CMS, which in turn prompts you to set up some account/project details so that you can import content.

In that world, rather than having a strict app/library dichotomy, those features would just be features of the template:

image

Not going to lie, aspects of this make me nervous. I don't like scaffolding tools to be too smart, because it requires you to make all your decisions up-front, and it's generally hard to change those decisions later (not so much because it's actually difficult, but because all the logic was hidden from view so it's hard to know how to, for example, add packaging logic to an app after the fact). But the bullet point list is compelling.

benmccann commented 2 years ago

it's hard to know how to, for example, add packaging logic to an app after the fact

For everything else, we've been promoting https://github.com/svelte-add/ which avoids this issue. Potentially the packaging command could be a Svelte Adder, but I'd be worried people wouldn't find it. We could make it so that create-svelte can run on an existing package and in that case it would run modification commands rather than creating a scaffold. I don't know that it's necessary for v1, but if we're planning on doing it down the road it might change how we build it now.

elliott-with-the-longest-name-on-github commented 2 years ago

I like @dummdidumm's proposal best out of all of these -- I think svelte-add is totally fine the way it is as a secondary feature-adder. Maybe we just call it out in the docs better -- I see a lot of "how add Tailwind" questions in the Discord channel.

Anyway, I think it makes sense to add a separate packaging option to create-svelte, as creating a SvelteKit app and creating a SvelteKit package are different enough that they deserve their own execution paths and both of them are native, core features to SvelteKit (unlike, say, Tailwind, which is just another add-on). As much as I love lists, I don't love the idea of create-svelte concerning itself with many other templates -- this seems like a good job for separate create-x packages, many of which could be created by the community. We could even create a doc page somewhere with the "official" Svelte starters along with top community ones.

elliott-with-the-longest-name-on-github commented 2 years ago

Whoops, apparently ctrl+enter closes an issue. My bad. 😕

ghostdevv commented 1 year ago

I think this has been resolved since the behaviour of create-svelte has been established - feel free to reopen if I was wrong :pray: