-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat: remove the postinstall script in favor of template prepare script #13304
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: cee9a29 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 |
|
preview: https://svelte-dev-git-preview-kit-13304-svelte.vercel.app/ this is an automated message |
|
|
||
| const cwd = process.cwd(); | ||
|
|
||
| const pkg = JSON.parse(fs.readFileSync('package.json', 'utf-8')); |
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 this correct? this assumes the vite command is always run with cwd being the sveltekit app.
As a user i'd also prefer if this was silencable without adding a prepare script.
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.
Can you run it any other way? I remember those PRs trying to allow you to configure the cwd and you commenting that we should reject those
Any ideas on how you'd want to silence it?
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.
for silencing it i guess going through onwarn would help.
One way to avoid false positives at least would be to check if @sveltejs/kit was in devDependencies.
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.
regarding control over cwd this was a genuine question. If we deem it appropriate to have this as a constraint (i think other parts of our code might also rely on it) then this method here is the easiest. If not, we'd have to go through vite root in configResolved.
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.
okay, I put it through onwarn
This is SvelteKit, so you kind of have to have @sveltejs/kit in your dependencies. I'm not sure what such a check would be looking for
I added an explicit reference to cwd so that if we ever do make cwd configurable we won't overlook this as something to check - though it doesn't sound like we'll be doing that so I'm not too worried about it
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.
All that the prepare script is supposed to be doing is run sync, which will just avoid the annoying warning. People having existing projects will not see that run anyway, and people with new projects going through the CLI will have the prepare script setup for them. So I would just remove that warning entirely. It's not like there's any harm in not running sync in advance other than this Vite warning.
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.
okay. I removed the warning and instead filed #13326 and added extra details to the changeset
|
interesting that
|
|
Installing a package from Git does run its |
|
we could try to detect presence of svelte-package in devDependencies and not log then. Alternatively if this didn't work in pnpm for a long time and we update the template for new apps, do we really need this warning? It'll sort itself after first call of |
Hmm. That would affect Svelte libraries published via git for consumption in non-SvelteKit projects. I guess in that case they can just remove the
I expect they'd still want
It is true that missing it doesn't break anything. But it's a nicer experience with it present and I feel most people aren't going to know they should add it now or even take the time to Google why all of a sudden they're getting a new warning. It feels a lot nicer if we can help people migrate |
|
Strange, I'm still seeing the warning, despite trying both $ pnpm -v
10.0.0
$ grep '@sveltejs/kit' package.json
"@sveltejs/kit": "^2.16.0",
$ grep prepare package.json
"preprepare": "svelte-kit sync",
"prepare": "svelte-kit sync",
$ rm -rf .svelte-kit/
$ pnpm run dev
> admin-platform@0.0.1 dev /Users/ryansobol/Projects/ryansobol/admin-platform
> vite dev
▲ [WARNING] Cannot find base config file "./.svelte-kit/tsconfig.json" [tsconfig.json]
tsconfig.json:2:12:
2 │ "extends": "./.svelte-kit/tsconfig.json",
╵ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
VITE v6.0.7 ready in 983 ms
➜ Local: http://localhost:5173/
➜ Network: use --host to expose
➜ press h + enter to show help |
|
Is this something all existing kit projects should add, or just for ones expected to use / potentially use pnpm 10? |
|
@ryansobol it looks like a bug in pnpm. Hopefully pnpm/pnpm#8989 should fix it @cloudymeatball all existing kit projects should add that line to their |
|
I get this error after upgrading to error during build:
[vite-plugin-sveltekit-compile] ENOENT: no such file or directory, open '/Users/username/Personal/project/.svelte-kit/output/server/undefined'
at Object.readFileUtf8 (node:internal/fs/sync:25:18)
at Object.readFileSync (node:fs:441:19)
at file:///Users/username/Personal/project/node_modules/@sveltejs/kit/src/exports/vite/build/build_server.js:56:24
at Array.forEach (<anonymous>)
at build_server_nodes (file:///Users/username/Personal/project/node_modules/@sveltejs/kit/src/exports/vite/build/build_server.js:48:5)
at Object.handler (file:///Users/username/Personal/project/node_modules/@sveltejs/kit/src/exports/vite/index.js:919:5)
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
at async PluginDriver.hookParallel (file:///Users/username/Personal/project/node_modules/rollup/dist/es/shared/node-entry.js:19858:17)
at async file:///Users/username/Personal/project/node_modules/rollup/dist/es/shared/node-entry.js:20860:13
at async catchUnfinishedHookActions (file:///Users/username/Personal/project/node_modules/rollup/dist/es/shared/node-entry.js:20282:16) |
No. It would be related to #13068 |
|
@benmccann, thanks a lot! Removing |
NOTICE: users should add
"prepare": "svelte-kit sync"to their projects.prepreparemay also be used in favor ofprepare, but is not currently supported on all systems. E.g. users of pnpm should preferprepareuntil pnpm/pnpm#8989 is merged and releaseThis solves multiple problems. Firstly, the
postinstallscript doesn't run on any version of pnpm today because pnpm caches side-effects and they get executed only once ever. Secondly, with pnpm 10 thepostinstallscripts of dependencies do not run by default and need to be enabled on a package-by-package basis