Skip to content

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

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

Closed
WillsterJohnson opened this issue Apr 26, 2022 · 11 comments
Closed
Labels
bug Something isn't working p2-nice-to-have SvelteKit cannot be used by a small number of people, quality of life improvements, etc.
Milestone

Comments

@WillsterJohnson
Copy link

WillsterJohnson commented Apr 26, 2022

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, '-' );
}
@Rich-Harris Rich-Harris added the bug Something isn't working label Apr 26, 2022
@Rich-Harris Rich-Harris added this to the 1.0 milestone Apr 26, 2022
@Mlocik97
Copy link
Contributor

Mlocik97 commented Apr 27, 2022

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
Copy link
Contributor

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:

  • If the package is scoped (i.e. of the form @scope/package), trigger another dialog asking for the directory location (with default/blank answers setting the directory to exactly the provided package name). Some people might want to keep their directory structure flat, storing @scope/package in a directory scope-package, for example.
  • Just let people deal with the fact that their scoped package will reside in @scope/package when the tool runs and let them rename/move stuff around if they want. (Edge case: If the directory @scope already exists in the current directory, would the current script fail? Should check.)

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

@WillsterJohnson
Copy link
Author

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
Copy link
Contributor

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

@Mlocik97
Copy link
Contributor

Mlocik97 commented Jun 5, 2022

vitejs/vite#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.

@benmccann benmccann added the p2-nice-to-have SvelteKit cannot be used by a small number of people, quality of life improvements, etc. label Jul 19, 2022
@dummdidumm
Copy link
Member

The discussion in #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

  • add private: true to the package.json in case the user wants an app
  • add svelte2tsx and typescript unconditionally to the package.json because that's needed for generating types. Right now you get an error the first time you try to do that
  • only add the package command to package.json if we want to create a lib
  • adjust the tsconfig so it uses the new node module resolution algorithm which has mandatory file endings. This is needed because the result is ESM and leads to Ensure .js extensions are present with Typescript 5 cli#205
  • scaffold a slightly different, more package-focused template

@Rich-Harris
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

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

@Rich-Harris Rich-Harris modified the milestones: 1.0, whenever Aug 27, 2022
@ghostdevv
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working p2-nice-to-have SvelteKit cannot be used by a small number of people, quality of life improvements, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants