Skip to content
This repository has been archived by the owner on Aug 11, 2022. It is now read-only.

Files generated by "prepublishOnly" script are not published during "npm publish". #15147

Closed
1 task done
vovacodes opened this issue Dec 5, 2016 · 21 comments
Closed
1 task done
Labels

Comments

@vovacodes
Copy link

vovacodes commented Dec 5, 2016

I'm opening this issue because:

  • npm is doing something I don't understand.

What's going wrong?

If you have some build step defined in prepublishOnly script, the files produced by it are not included in the tar and aren't published when npm publish is called

How can the CLI team reproduce the problem?

Clone https://github.com/wizardzloy/prepublish-bug , try to publish it, check that lib folder is not published even though it's generated during prepublishOnly phase

supporting information:

  • npm -v prints: 4.0.3
  • node -v prints: v6.5.0
  • npm config get registry prints: https://registry.npmjs.org/
  • Windows, OS X/macOS, or Linux?: macOS Sierra
@legodude17
Copy link
Contributor

I don't know if this is a bug or an expected result.
Pinging @kenany and @othiym23

@kenany
Copy link
Contributor

kenany commented Dec 7, 2016

Probably a bug.

@zkat zkat added bug support and removed bug labels Dec 9, 2016
@zkat
Copy link
Contributor

zkat commented Dec 9, 2016

Sorry for the back and forth bug label. That's what I get for triaging at night.

This is working as expected. prepubishOnly is not meant to be used for build steps. This lifecycle runs after tarball creation, right before we do the final upload. If you want to have a build step, please use prepare instead, as of npm@4.

@zkat zkat closed this as completed Dec 9, 2016
@vass-david
Copy link

The problem we (me and @wizardzloy) are solving here is that we use "peerDependencies", which are not installed with npm i, but are necessary in one of build tasks - webpack build (transpilations via babel used in es6 projects are ok). Having build in "prepare" means that npm i will fail and will break our CI. What solution do you suggest for this situation? Or are we doing something wrong?

@zkat
Copy link
Contributor

zkat commented Dec 9, 2016

Why not just install that dependency as a devDependency?

@vass-david
Copy link

I was thinking about that too, but I don't like the idea of having stuff in two places (peerDependencies and devDependencies). And IMHO devDependencies are tools for building, like bundling, transpilation, tests, lint and different similiar stuff that is not directly imported in source files. Also in npm docs there in devDependencies section there is:

If someone is planning on downloading and using your module in their program, then they probably don't want or need to download and build the external test or documentation framework that you use.

In this case, it's best to map these additional items in a devDependencies object.

... and I think it should remain like that. We definitely want target project to have these dependencies installed and use their own version of these packages. That's why the most logical place is only "peerDependencies" list here.

@vovacodes
Copy link
Author

@zkat what the prepublishOnly was designed for and why is it called after tarball creation? Because it is quite important not to introduce a new source of confusion for developers, like the one with prepublish invocation, that prepublishOnly was supposed to solve

@zkat
Copy link
Contributor

zkat commented Dec 9, 2016

@wizardzloy prepublishOnly was explicitly designed just to tell users "heeey, we're about to upload the tarball". That's it. We expect it to be quite rare. prepare is the one that 99.999% of people probably want when they want a prepublish event. prepare is when any build steps should happen.

@vass-david what is it that this peerDependency-dependent task is doing? Creating lib definitely feels like a build step. What is the peerDependency and why does it break your CI?

@vovacodes
Copy link
Author

@zkat @vass-david thepeerDependencies in our project are the ones that are used in our compiled commonjs output and should be provided by the app that uses this library. But we also generate umd bundle where those peerDependencies are included, that part is done in prepublish script and it fails during npm install. My idea was to simply install peerDependencies in prepublish, that should solve the issue

@zkat
Copy link
Contributor

zkat commented Dec 10, 2016

@wizardzloy: the intention behind npm install is to bring your package to a place where it works, and prepare is the script intended for that.

How about this: Install your peerDependencies as BOTH peerDeps and devDeps. npm install shouldn't error in that case, because it should have everything locally installed that it needs to have installed. Then, if you want to do a local npm install _as a developer, but you don't want to go through the slower process of doing all that compilation, just run it as npm install --ignore-scripts, which will skip prepare` entirely, but will likely still give you a working dependencies tree.

@vovacodes
Copy link
Author

@zkat thanks for your help, this could be a solution indeed!

@zkat
Copy link
Contributor

zkat commented Dec 10, 2016

@wizardzloy awesome! Cheers! :D

@Wildhoney
Copy link

Sorry to comment on a closed ticket. However, the prepublishOnly script running post-tarball creation is somewhat surprising to me – I fail to see the benefit of "heeey, we're about to upload the tarball".

In my case I want to run a script on npm publish but not on npm install. How would you folks recommend going about this?

I can't seem to achieve that because:

  • npm run prepare runs on both install and publish.
  • npm run prepublishOnly runs post-tarball.

@zkat
Copy link
Contributor

zkat commented Jan 13, 2017

@Wildhoney I think the important question here is what the use case for something like this would be. In what usecase do you want to affect the potential contents of a tarball only when you're intending to upload it?

@Wildhoney
Copy link

Wildhoney commented Jan 13, 2017

@zkat on npm publish I move the files from the src directory to the root directory so when other people use the module they import 'module/file' rather than import module/src/file, however on npm i I don't want this mv to occur. For the repository I want to maintain all of the files in the src directory.

I have multiply "entry" files, and the main field in the package.json is fairly weak in it only supports one default entry.

// I can either configure "main" or move file to root.
import a from 'module';

// As far as I know, only supported by a move to root operation.
import b from 'module/b';

@vovacodes
Copy link
Author

@zkat I also was thinking about this issue lately, as we know prepublishOnly will be replaced with prepublish again in npm@5.x.x so it looks logical to me for it to work similarly to how it works now, i.e. modify content of a tarball before publishing it.

@clayne11
Copy link

clayne11 commented Feb 14, 2017

This is incredibly confusing. My understanding was that prepublishOnly would run before tarball creation so that I can build the package before publishing it to npm. There are a lot of reason why you might want a step to run before tarball creation but not during install.

One example is that on my CI server I don't want to run want to prepare the package for publishing, I just want to download it and run tests. My prepublish step takes 20 minutes on a CI server and just wastes build time for no reason.

This change doesn't make any sense to me in terms of making npm less confusing and also doesn't add a way to run a script only on publishing which I think is what 99% of people were hoping for with this change.

robmcguinness pushed a commit to Availity/availity-angular that referenced this issue Mar 9, 2017
@bfreis
Copy link

bfreis commented Mar 23, 2017

@clayne11 +1

This just makes absolutely no sense.

In the last few weeks I spent countless hours having to publish my packages twice, because I couldn't understand why my version numbers were increasing but my code wasn't updating - I was using prepublishOnly to run babel and other stuff.

I also spent a few hours today writing tests to send to the NPM Support team about this, until I inspected the verbose log of npm publish and realized it was running prepublishOnly after the tarball was generated. I then wrote back to support saying that I found the bug on npm. I was like, this is clearly a bug. Why on earth would someone call a phase "prepublishOnly" if any tasks you run on that phase are discarded wrt the data being published??

I'm definitely part of the 99% of people that @clayne11 describes. And that's very likely way more than 99%...

On one hand, I'm glad to see it going away. On the other hand, I'm concerned about stability of the tool - things coming and going are a bit of a concern.

Thanks
Bruno

@zkat
Copy link
Contributor

zkat commented Mar 23, 2017

Y'all who wanted to use prepublish should just use prepare. It has exactly the same semantics and is intended for builds. It's kind of ironic that this is so confusing because the whole reason for this change was because we kept getting people in our issue tracker who were very confused that prepublish ran whenever you did npm install.

That said, yeah, we're probably gonna get prepublishOnly to run before pack, probably as part of npm@5. It might be sooner cause it could be considered a bug.

@kyasbal
Copy link

kyasbal commented Apr 10, 2017

@clayne11 +1

I think there are 2 meanings of semantics in prepublish. And that is cause of this misunderstanding.

I had confusing about prepublish when I use npm@3.
In my case, prepublish script contains test job , lint job and bundling job. Because of this script, I have not to afraid of publishing buggy product. And, I can make sure that the published product was bundled from new scripts.

But, at npm@3, prepublish was fired during npm install. I didn't want to run prepublish during install script. Because these scripts are not for prepareing modules in the environment. I just used them for validating releasing packages.

I guess there was 2 semantics when I use prepublishOnly. These are "I need to prepare(build natively? or something very restricted in local environment) for installed modules" or "I need to validate and build releasing version of the package".

In the 2 semantics, I guess the former one is now solved by prepare. And I believe the later one should be solved by prepublishOnly. But actually it doesn't.

mathewhawley pushed a commit to suitejs/suitejs that referenced this issue Jul 22, 2017
As this repository is developed in npm@5, the 'prepublish' hook was being
used to perform PRE-PUBLISH tasks (e.g. lint, test, transpile). However, it will
still be executed via 'npm install' on older versions of NPM by a consumer of
the package. This will cause the package installation to fail, as development
dependencies/tasks are not intended to be downloaded/executed on 'install'.

There is a lot of confusion around this:
npm/npm#16685
npm/npm#15147

At the time of writing, there is no lifecycle hook available to perform a task
ONLY prior to publishing, that is backwards compatible.

A manual 'validate' script has been created to be used in it's place.

Any publishing activity will need to make sure this script is run across all
packages before the 'lerna publish' script is run. It should also be used in
place of 'lerna bootstrap'.
mathewhawley pushed a commit to suitejs/suitejs that referenced this issue Jul 22, 2017
As this repository is developed in npm@5, the 'prepublish' hook was being
used to perform PRE-PUBLISH tasks (e.g. lint, test, transpile). However, it will
still be executed via 'npm install' on older versions of NPM by a consumer of
the package. This will cause the package installation to fail, as development
dependencies/tasks are not intended to be downloaded/executed on 'install'.

There is a lot of confusion around this:
npm/npm#16685
npm/npm#15147

At the time of writing, there is no lifecycle hook available to perform a task
ONLY prior to publishing, that is backwards compatible.

A manual 'validate' script has been created to be used in it's place.

Any publishing activity will need to make sure this script is run across all
packages before the 'lerna publish' script is run. It should also be used in
place of 'lerna bootstrap'.
@dominictarr
Copy link

adding to what @kyasbal-1994 said, I've just noticed that prepublishOnly ignores the exit code of the script, so for example I used to use prepublish to run "npm ls && npm test" which prevented me acidentially publishing broken stuff, other people run linters, etc. if prepublishOnly runs after pack, and can't effect whether or not publishing happens... I'm really not sure what prepublishOnly is even useful for?

pirelenito added a commit to klarna/higher-order-components that referenced this issue Aug 2, 2017
The `prepublishOnly` lifecycle is just so confusing, it doesn’t do at all what you would expect, but instead:

“...prepubishOnly is not meant to be used for build steps. This lifecycle runs after tarball creation, right before we do the final upload. If you want to have a build step, please use prepare instead, as of npm@4.”


Here is an issue and discussion about how it makes no sense: npm/npm#15147 (comment)
jessepinho added a commit to groupon/tiquette that referenced this issue Sep 20, 2017
mythmon added a commit to mythmon/pioneer-utils that referenced this issue Nov 1, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

10 participants