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

Run install scripts in pnpm@10 #388

Open
hyunbinseo opened this issue Jan 8, 2025 · 17 comments · May be fixed by #409
Open

Run install scripts in pnpm@10 #388

hyunbinseo opened this issue Jan 8, 2025 · 17 comments · May be fixed by #409
Labels
enhancement New feature or request pkg:create

Comments

@hyunbinseo
Copy link
Contributor

In pnpm@10, install scripts must be individually enabled.

Lifecycle scripts of dependencies are not executed during installation by default!

If not, the following message will be logged.

The following dependencies have build scripts that were ignored: @sveltejs/kit, esbuild

To allow the execution of build scripts for these packages, add their names to "pnpm.onlyBuiltDependencies" in your "package.json", then run "pnpm rebuild"

What if this field is added to the package.json if the user selects pnpm?

◇  Which package manager do you want to install dependencies with?
│  pnpm
{
  "pnpm": {
    "onlyBuiltDependencies": [
      "@sveltejs/kit",
      "esbuild"
    ]
  }
}
@manuel3108 manuel3108 added bug Something isn't working pkg:create labels Jan 9, 2025
@benmccann
Copy link
Member

This is probably the best solution for now if pnpm is being used

I do think it'd be nice to be able to set this globally in addition to being able to set it on a per-project basis: pnpm/pnpm#8891

@benmccann
Copy link
Member

benmccann commented Jan 9, 2025

The postinstall hook isn't running for me in pnpm 9 anyways: sveltejs/kit#5779 (comment)

Screenshot from 2025-01-09 11-31-04

@hyunbinseo what does pnpm 10 do if you don't set this option?

@hyunbinseo
Copy link
Contributor Author

hyunbinseo commented Jan 11, 2025

This is the full log using pnpm@10:

┌  Welcome to the Svelte CLI! (v0.6.11)
│
◇  Where would you like your project to be created?
│  ./
│
◇  Which template would you like?
│  SvelteKit minimal
│
◇  Add type checking with Typescript?
│  Yes, using Typescript syntax
│
◆  Project created
│
◇  What would you like to add to your project? (use arrow keys / space bar)
│  prettier, eslint
│
◇  Which package manager do you want to install dependencies with?
│  None
corepack use pnpm@10
Installing [email protected] in the project...

Packages: +187
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Progress: resolved 227, reused 177, downloaded 10, added 187, done

devDependencies:
+ @eslint/compat 1.2.5
+ @eslint/js 9.18.0
+ @sveltejs/adapter-auto 3.3.1
+ @sveltejs/kit 2.15.2
+ @sveltejs/vite-plugin-svelte 4.0.4 (5.0.3 is available)
+ eslint 9.18.0
+ eslint-config-prettier 9.1.0
+ eslint-plugin-svelte 2.46.1
+ globals 15.14.0
+ prettier 3.4.2
+ prettier-plugin-svelte 3.3.2
+ svelte 5.17.3
+ svelte-check 4.1.3
+ typescript 5.7.3
+ typescript-eslint 8.19.1
+ vite 5.4.11 (6.0.7 is available)

The following dependencies have build scripts that were ignored: @sveltejs/kit, esbuild
To allow the execution of build scripts for these packages, add their names to "pnpm.onlyBuiltDependencies" in your "package.json", then run "pnpm rebuild"

I don't think the svelte-kit sync runs even with the following key-value set:

◇  Which package manager do you want to install dependencies with?
│  None
{
  "pnpm": {
    "onlyBuiltDependencies": ["@sveltejs/kit", "esbuild"]
  }
}
corepack use pnpm@10

At this point, .svelte-kit directory is not created.

@zkochan
Copy link

zkochan commented Jan 12, 2025

You need to have that setting in your package.json before you run install. If you added it after install, you should've run pnpm rebuild to run the scripts of the dependencies.

What if this field is added to the package.json if the user selects pnpm?

Yes, that is the right solution.

@hyunbinseo
Copy link
Contributor Author

You need to have that setting in your package.json before you run install.

That is what I've done in the above comment. (in the latter part)

  1. run pnpm dlx sv create
  2. add onlyBuiltDependencies
  3. run corepack use pnpm@10 - which installs the packages
  4. the .svelte-kit directory is not created

@manuel3108
Copy link
Member

I hate windows! I'm unable to update my pnpm to version 10.

Is anyone able to confirm that this works as expected #395 ?

pnpx https://pkg.pr.new/sveltejs/cli/sv@395 create

@manuel3108 manuel3108 added enhancement New feature or request and removed bug Something isn't working labels Jan 12, 2025
@zkochan
Copy link

zkochan commented Jan 12, 2025

It seems to work. I checked the "ignoredBuilds" array inside "node_modules/.modules.yaml" and it doesn't contain any elements. So, the dependencies were built. (We will also add a separate command for listing any ignored builds)

@benmccann
Copy link
Member

It's not working because of pnpm's side-effects cache. We'd also need to disable that in the .npmrc. I'm not sure if we can disable it only for SvelteKit. I think it's all or nothing. I'm not sure what esbuild's postinstall script does and if it is expected to be run multiple times or how much doing so might slow down subsequent builds

@zkochan
Copy link

zkochan commented Jan 12, 2025

What is the problem with pnpm's side-effects cache? I don't think I have noticed any issues with side effects cache and esbuild. We have esbuild in pnpm's monorepo as a dependency with side-effects enabled.

@benmccann
Copy link
Member

The problem isn't with esbuild, but with SvelteKit. SvelteKit's postinstall needs to run in a clean checkout or after a git clean and right now it never runs

@zkochan
Copy link

zkochan commented Jan 12, 2025

Could your template also add a prepare script that runs the required script from sveltekit? That feels like the right way to do it as it looks like sveltekit's postinstall is not intended for building sveltekit but for preparing some things in the dependent project. This is somewhat similar to husky, which configures git-hooks. You should run husky from a prepare script as well.

Alternatively, maybe we could support some field in package.json that would tell pnpm not to use side-effects cache for the given package (it would be a field in sveltekit itself). IMO the prepare script would be better but we can discuss it with more people.

@benmccann
Copy link
Member

In terms of lifecycle, postinstall seems more fitting as we don't need to do anything before publish, but only after install. Does prepare skip the side effects cache?

We could potentially move the postinstall script from SvelteKit to the user's project. Would doing so mean that we don't need to set onlyBuiltDependencies in that case? Would have it in the user's project allow it to skip the side effects cache?

@zkochan
Copy link

zkochan commented Jan 12, 2025

Yes, if you run your dependency's script from the prepare script of the project, then it won't use any side-effects cache, and it won't require changes to onlyBuiltDependencies

@benmccann
Copy link
Member

What about if we run it from the project's postinstall? Would that use the side-effects cache, but not require changes to onlyBuiltDependencies?

@zkochan
Copy link

zkochan commented Jan 12, 2025

It will work from any script of the project. So, postinstall will also not use side-effects cache and require no changes to onlyBuiltDependencies.

Also, this solution will work with every package manager, not just pnpm.

@benmccann
Copy link
Member

Thank you for the help and advice. I've sent a PR to remove SvelteKit's postinstall script: sveltejs/kit#13304

I recommended users to add a prepare script. This is better for users publishing libraries as it won't be run by people installing the library the way a postinstall script would

If sveltejs/kit#13304 is merged, we should update the template in this repo to contain a prepare script

@benmccann
Copy link
Member

I put together a PR, which actually uses the preprepare script (#409). This works perfectly on npm where it runs on local install, but not on publish, which is exactly what we want. However, during testing I discovered that pnpm unfortunately has a different behavior than npm here. @zkochan I filed pnpm/pnpm#8987 in case there's a chance it's considered a bug that pnpm's behavior differs from npm's. You can see npm's behavior both in the reproduction I provided and in npm's docs, which I linked to in the issue. Thanks again for all your help here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request pkg:create
Projects
None yet
4 participants