Skip to content
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

svelte-kit package should automatically include Svelte as a peerDependency #3023

Closed
cdellacqua opened this issue Dec 11, 2021 · 10 comments
Closed
Labels
documentation Improvements or additions to documentation pkg:svelte-package Issues related to svelte-package
Milestone

Comments

@cdellacqua
Copy link

Describe the bug

I used svelte-kit package/npm run package to create a simple library that provides some Svelte stores.

The package command prepared a folder containing all the library files, but the generated package.json did not include Svelte as a peerDependency, even thought the library code depends on the Svelte runtime environment (in my case the generated bundle contains import statements targeting 'svelte/store').

The generated library only works in an environment where Svelte has already been installed and won't warn the user in case of breaking changes to the runtime. For example, suppose that a year from now Svelte 4 were to be released, all libraries that don't include "svelte": "^3.44.0" as a peerDependency may silently break without the usual warning from npm.

Reproduction

One can reproduce the issue using the default demo project:

# you can just press Enter and go with the default options
npm init svelte@next my-app
cd my-app
# the following are needed for generating a package
npm i -D typescript svelte2tsx
npm run package

# and then
cat package/package.json

The package/package.json file contains Svelte as a devDependency, thus resulting in an untracked dependency on the users' side.

Logs

No response

System Info

System:
    OS: Linux 5.15 Arch Linux
    CPU: (16) x64 AMD Ryzen 7 5700U with Radeon Graphics
    Memory: 11.79 GB / 15.03 GB
    Container: Yes
    Shell: 3.3.1 - /usr/bin/fish
  Binaries:
    Node: 17.1.0 - /usr/bin/node
    npm: 8.1.3 - /usr/bin/npm
  Browsers:
    Chromium: 96.0.4664.45
    Firefox: 94.0.2
  npmPackages:
    @sveltejs/adapter-auto: next => 1.0.0-next.3 
    @sveltejs/kit: next => 1.0.0-next.201 
    svelte: ^3.44.0 => 3.44.2

Severity

annoyance

Additional Information

No response

@cdellacqua cdellacqua changed the title svelte-kit package should include Svelte as a peerDependency svelte-kit package should automatically include Svelte as a peerDependency Dec 11, 2021
@dummdidumm dummdidumm added the pkg:svelte-package Issues related to svelte-package label Dec 11, 2021
@dummdidumm
Copy link
Member

dummdidumm commented Dec 11, 2021

I tend to agree, although it's prefectly valid to create packages that don't depend on Svelte directly, for example packages that only provide actions (use:xxx), in that case it would be a "false positive".
Regarding implementation: Where to put this? Right now the contents of the package.json are respected, and putting it in there would make it visible and explicit. But it would be confusing for people writing Apps why there's a peerDependency in there. If it's not put in there, which version to set as peer dep? I'd use the one of Svelte in dev deps.
This and other issues like #2951 make me think we should have an option during the setup to ask if one plans on publishing a lib - in that case, an explicit peer dep in package.json wouldn't be confusing.

@cdellacqua
Copy link
Author

An action can't be assumed to work correctly on different major versions of Svelte, because the syntax or even some assumptions on components lifecycle could change in the future. So I don't think it would be a false positive and Svelte should still appear as a peerDep, even if the Svelte runtime libs are not used in the code.

I get your point about the version that should be specified. Using the same version as the devDeps is a possibility and should guarantee compatibility, but at the same time this could be problematic for adoption, because a new library would naturally use the latest version of Svelte while a lot of already existing projects would not, thus resulting in a lot of peerDep warnings from npm.

I have to strategies in mind:

  1. npm run package should fail with a clear error stating that Svelte should be added as a peerDep in the root package.json before running package;
  2. npm run package should prompt the developer with a question that asks something like "svelte should be added as a peerDep to your project. Which version should be used as a peerDep?", possibly with a default to the devDep version.

These two strategies would only be used if Svelte isn't already a peerDep.

I tend to think that the first strategy is better, because it forces the developer to make a conscious decision.

@bluwy
Copy link
Member

bluwy commented Dec 12, 2021

I agree with @dummdidumm. The svelte peer dep isn't necessary for actions library, they can be used generically and are not bound to Svelte. So I don't think adding svelte peer dep automatically is ideal here.

Relying on dev dep version would be a bit fragile too. Suppose you want to test your library with the latest svelte version, but still supporting an older svelte version, the peer dep and dev dep version would be different. Bumping svelte peer dep version is also a major change in semver, so it would be rare if someone wants to do it.

I think @dummdidumm's suggestion of doing that during setup would be the best way. That should prompt the developer to be aware of the peer dep and manage it.

@ignatiusmb
Copy link
Member

Do keep in mind that svelte-kit package can also be used to build libraries that are fully TypeScript / JavaScript based that are not tied to Svelte at all, and only uses SvelteKit to build the documentation site. In that case, authors would need to be fully aware and in control of what should and shouldn't be added as dependencies, either as prod/dev/peer. Feels like this suits better as a documentation issue and should be placed there rather than prompts or errors.

@cdellacqua
Copy link
Author

@ignatiusmb you made a good point about making non-Svelte libraries.

Could we create an FAQ section related to libraries made using sveltekit and display the link to the developer during init, so that they are aware of this, even if it's their first library?

$ npm init svelte@next my-app
...questions

Done. Next steps
cd my-app
npm install


Are you building a library? You can read the FAQs here: https://www.example.com/sveltekit-lib-faqs.html

The FAQs could then point out the different approaches to peerDeps (actions vs runtime vs etc) among other useful distilled information

@ignatiusmb
Copy link
Member

Where do we draw the line between SvelteKit packaging and npm's packaging/publishing? I don't think it is in our scope to explain how npm works. There would be a lot of follow-up questions if we do put them in the FAQ, and it would become a docs on its own by that point.

@cdellacqua
Copy link
Author

It's seems to me that the moment a command like svelte-kit package exists its behaviour should be thoroughly explained, even considering the fact that this command is mangling the original package.json, automating stuff like the "exports" field.

I don't think it's a problem to have a somewhat redundant documentation that could be found elsewhere on the web about how npm packaging works, considering that svelte kit is using a subset of all the available functionalities.

It could also be a great way to introduce new developers to the fragmented world of JavaScript libraries/bundlers etc.

@dummdidumm
Copy link
Member

Sounds like something for the documentation rework we will be doing at some point. If we decide to go multi-page, we will have enough space for more in-depth explanations.

@dummdidumm dummdidumm added the documentation Improvements or additions to documentation label Jan 27, 2022
@Rich-Harris Rich-Harris added this to the post-1.0 milestone May 12, 2022
@ivanhofer
Copy link
Contributor

I think an alternative approach would be to allow the lib folder to contain a package.json file. If someone wants to specify peerDependencies or "real" dependencies he can do it there. The entries added could override the ones from the root package.json. This would give package authors more control.

@benmccann
Copy link
Member

I'm going to consider this closed by #7685

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation pkg:svelte-package Issues related to svelte-package
Projects
None yet
Development

No branches or pull requests

7 participants