Closed abeforgit closed 3 years ago
Pretty sure prepare
runs after someone installs diary – which would be annoying and/or throw errors of its own
From my understanding it only runs when installing from a github repo. It also runs when running npm install
in the diary repo, when you're working on it. It also runs when running npm publish
. But I don't think it runs if you just install diary from the registry.
Though I must say this topic is incredibly confusing and the documentation doesn't word it clearly, I'll look into it some more
Yeah, no idea TBH. If it's just a matter of getting it to run before test
then I'd be explicit and add a pretest
script. No reason to try to do something fancier esp if not well understood :)
Also, looks like there are other, unrelated changes in this PR.
No, the usecase is allowing people to use a fork as a dependency. As it stands, forking and installing the fork does not work, since the build step is not being executed. prepare
is the only way to do this.
I suppose the choice could be made to just not support that kind of install, but I can attest first hand it can add quite a hurdle to adoption of this library, so I wanted to make the process smoother for future users.
As for the unrelated changes, my bad, will clean them up
Ok so I've created a dummy npm package to test out the behavior of prepare. https://github.com/abeforgit/test-npm-run-prepare https://www.npmjs.com/package/test-npm-run-prepare
As you can see, the prepare script runs build.js, which always throws an error. I've published version 1.2.0 with the npm publish --ignore-scripts
command, which publishes without running the scripts.
The master branch of the repo is on that same 1.2.0 commit.
now to test this I made another dummy npm project and did the following:
npm install abeforgit/test-npm-run-prepare
-> this crashed, as expected, cause the build script is run and throws an error.
npm install test-npm-run-prepare
-> this installs without any problems.
rm -rf node_modules
rm package-lock.json
npm install
-> also installs without any problems
Combined with my understanding of the docs, I am convinced prepare does not run on an installation from the registry. It's not much, but there's also this comment on the npm forums: https://npm.community/t/npm-6-12-0-npm-ci-doesnt-run-prepare-hook-when-package-json-contains-private-true/10452
Hi there @abeforgit
Thank you again so much for contributing and using my thing! 🙏🏻 Truthfully, you've been nothing short of amazing.
Re this change though — and with all due respect. I'm not really keen on the idea of optimizing for forks, much rather have these things contributed back so others can also benefit from it.
I don't know much about the prepare
script type, but from what I can see it Runs on local npm install without any arguments as well, which isnt something id want to have happen. I'm not 100% sure if pnpm
would run that as well, but the idea is building for me is just something i want to logically do before a publish, and was honestly just there for me in case im negligent and forget to build before a publish.... That being said though, not an excuse!
If i simply removed the script prepublishOnly
would this suffice for you?
Yeah that wording on the npm docs is just horrible... its so confusing cause they don't specify where you run npm install without arguments...
As you can see though from my experiments above, it does not run when you run npm install
without arguments in the consuming app. It does run when you run it in the diary repo itself, aka for the developers/contributors. I don't really see why that would be an issue, since you need to build before publishing anyway, if the build ever fails thats a blocking issue regardless when it happens. I'd still really like to find some official statement about this, but so far my searches have come up blank, very frustrating. But I'd encourage you to look into the test repo I made and try it out for yourself.
As for not optimizing for forks, I can see your point, but consider that it is only through forks that people can contribute in the first place. Personally, if this change doesn't come through for example, I have to maintain a branch on my fork where the prepare script is setup, otherwise I simply cannot continue with this project as a dependency.
Now to be clear, maintaining a branch like that is entirely possible and not that hard, so I don't want to pressure anyone into accepting the PR, I just don't think any of the arguments against it are actually valid.
Removing the prepublishOnly
script would not do anything for me, and I'd strongly advise against doing so, I think its a good thing that you've automated the building of the project before publishing. The only reason I changed it to the prepare script instead of simply adding a prepare script is that prepare always runs when prepublishOnly also runs, so there's no need for the two scripts at once.
@abeforgit you make some good points — just curious, have you seen patch-package yet? It'll allow you to iterate far quicker, and solve that time-to-review metric; as well as not have to maintain a fork.
Going to close this one — this change isnt exactly something I'd want here. Personally when I install, I want it to install. Removing the prepublishOnly step as well, I should build before publishing.
Since building is necessary for any kind of work on the library, it's better to use prepare instead of prepublishOnly. I hit this usecase when trying to switch to using my diary fork as a dependency for my consuming app (we have to move too fast to wait for pr reviews).
Reference: https://docs.npmjs.com/cli/v7/using-npm/scripts
The main difference is that prepare is also ran when installing directly from a github repo.