This repository has been archived by the owner. It is now read-only.

resolving prepublish-on-install #10074

Closed
othiym23 opened this Issue Oct 22, 2015 · 71 comments

Comments

Projects
None yet
@othiym23
Contributor

othiym23 commented Oct 22, 2015

See also: #3059, #8402, #9722, #9733, probably many others.

Many, many people intensely dislike the fact that a lifecycle event named "prepublish" also gets run when a package is also installed for development, as this precludes the ability to have a set of validation checks that get run before publishing (and also because the name becomes confusingly inaccurate). The reasons for this behavior have been discussed, bikeshedded, and vehemently argued over elsewhere. There is a consensus that the current behavior is undesirable, and the CLI team agrees that this situation needs to be fixed. This is what we intend to do:

  1. Introduce a new prepare lifecycle event, which has the current behavior of the prepublish event – it runs before the package tarball is packed and uploaded to the registry during the publishing process, as well as when you run npm install (with no package name) after cloning a package, to prepare it for use (i.e. by transpiling source).
  2. Introduce a new prepublishOnly lifecycle event, which runs only at prepublish time.
  3. Add a new warning when the prepublish hook is used that the current behavior is deprecated, and will be removed at some point in the future.
  4. In a year or so, make a semver-major bump to npm and make prepublish's behavior match prepublishOnly.
  5. Either then or sometime after that, deprecate prepublishOnly and have just prepare and prepublish.

This should allow everyone to get the behavior they want, and will (eventually) result in a solution that is intuitive and unsurprising to everyone, and allows us to do so in about as gentle a way as possible for something so potentially disruptive.

Thoughts? Things we haven't considered? Parts 1-3 can (and will) be implemented as part of npm@3, probably over the next couple months, so it would be nice to get people's feedback relatively quickly.

@samccone

This comment has been minimized.

Show comment
Hide comment
@samccone

samccone Oct 22, 2015

👍 awesome, this is great

👍 awesome, this is great

@jasonrhodes

This comment has been minimized.

Show comment
Hide comment
@jasonrhodes

jasonrhodes Oct 22, 2015

\o/ niiiiiice

\o/ niiiiiice

@robertkowalski

This comment has been minimized.

Show comment
Hide comment
@robertkowalski

robertkowalski Oct 22, 2015

Member

+1 that is really good

Member

robertkowalski commented Oct 22, 2015

+1 that is really good

@fengmk2

This comment has been minimized.

Show comment
Hide comment
@fengmk2

fengmk2 Oct 22, 2015

Contributor

+1 great!

Contributor

fengmk2 commented Oct 22, 2015

+1 great!

@ahdinosaur

This comment has been minimized.

Show comment
Hide comment

👍 keen

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Oct 22, 2015

Contributor

Awesome, thanks!!

Contributor

Fishrock123 commented Oct 22, 2015

Awesome, thanks!!

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 22, 2015

Sounds great! The new prepare hook will be useful as well.

mgol commented Oct 22, 2015

Sounds great! The new prepare hook will be useful as well.

@dominictarr

This comment has been minimized.

Show comment
Hide comment
@dominictarr

dominictarr Oct 22, 2015

👍 💯 😄

👍 💯 😄

@mathieumg

This comment has been minimized.

Show comment
Hide comment
@mathieumg

mathieumg Oct 22, 2015

🎉

@othiym23 I imagine #8263 will be tackled separately from this?

🎉

@othiym23 I imagine #8263 will be tackled separately from this?

@JamesMGreene

This comment has been minimized.

Show comment
Hide comment
@JamesMGreene

JamesMGreene Oct 22, 2015

There is a god! 🙏

There is a god! 🙏

@SystemParadox

This comment has been minimized.

Show comment
Hide comment
@SystemParadox

SystemParadox Oct 22, 2015

Yes absolutely yes!

Yes absolutely yes!

@mtscout6

This comment has been minimized.

Show comment
Hide comment
@mtscout6

mtscout6 Oct 22, 2015

👍 Thanks for tackling this issue!

👍 Thanks for tackling this issue!

@AlastairTaft

This comment has been minimized.

Show comment
Hide comment
@AlastairTaft

AlastairTaft Oct 22, 2015

Brilliant 👍

Brilliant 👍

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Oct 23, 2015

Contributor

@othiym23 is there any chance that a tiny 1.x and 2.x patch could be put out, that detects the "prepublishOnly" and "prepare" scripts, and warns that the user should be using npm 3.something to get proper publish behavior? I know modifying 1.x is probably unlikely, but it would be very useful in helping encourage people to update to npm 3, and also ensuring they don't unknowingly miss a critical "before publish" step.

If not, then my only option would be to use in-publish inside "prepublish" for now - but then once "prepublish" does the right thing when your step 4 happens, I'd have to keep in-publish in there forever to ensure that 1.x, 2.x, and too-early-3.x users don't accidentally miss a publish step.

I kind of feel like prepublish should just be decommissioned and never replaced, rather than changing the semantics such that old npm running on an updated module does something surprising.

Steps 1, 2, and 3 seem wonderful to me - it's just steps 4 and 5 I'm concerned about. Instead, if they were a single step 4: In a year or so, make a semver-major bump to npm and remove prepublish, and have just "prepare" and "prepublishOnly". The name will be unfortunate, but I think, less bad than having now-era "prepublish" scripts conflict semantically with future-era "prepublish" scripts.

Contributor

ljharb commented Oct 23, 2015

@othiym23 is there any chance that a tiny 1.x and 2.x patch could be put out, that detects the "prepublishOnly" and "prepare" scripts, and warns that the user should be using npm 3.something to get proper publish behavior? I know modifying 1.x is probably unlikely, but it would be very useful in helping encourage people to update to npm 3, and also ensuring they don't unknowingly miss a critical "before publish" step.

If not, then my only option would be to use in-publish inside "prepublish" for now - but then once "prepublish" does the right thing when your step 4 happens, I'd have to keep in-publish in there forever to ensure that 1.x, 2.x, and too-early-3.x users don't accidentally miss a publish step.

I kind of feel like prepublish should just be decommissioned and never replaced, rather than changing the semantics such that old npm running on an updated module does something surprising.

Steps 1, 2, and 3 seem wonderful to me - it's just steps 4 and 5 I'm concerned about. Instead, if they were a single step 4: In a year or so, make a semver-major bump to npm and remove prepublish, and have just "prepare" and "prepublishOnly". The name will be unfortunate, but I think, less bad than having now-era "prepublish" scripts conflict semantically with future-era "prepublish" scripts.

@othiym23

This comment has been minimized.

Show comment
Hide comment
@othiym23

othiym23 Oct 23, 2015

Contributor

is there any chance that a tiny 1.x

No. The book is closed on npm@1. There is going to be one more release of it that warns people to upgrade to at least npm@2 and that early errors when users try to install scoped packages, strictly so that we can transition to npm@2 in Node 0.10 LTS.

and 2.x

Probably, although we'd be more likely to cherry-pick 1-3 onto the 2.x branch, or add a simple deprecation warning to uses of prepublish that its behavior is changing. That's an LTS discussion, and so will probably involve @zkat and the Node LTS working group.

The reason to wait a year between steps 3 and 4 is twofold - to give developers a chance to migrate to prepare and prepublishOnly, and to give everyone a chance to upgrade to newish versions of npm. There's no reason for people to stay on old versions of npm, even if for whatever reason they can't move off of old versions of Node. We've put a considerable amount of work into keeping npm backwards compatible all the way to Node 0.8. This is especially true for publishers, who need to keep reasonably up to date just so they have an npm that works with the current registry architecture.

I don't think it's especially onerous to include "in order to publish this package, you must be running at least npm@3.4.5" in CONTRIBUTING.md. If you do want to go the extra mile and support old versions of npm, having a dependency on in-publish doesn't seem terrible, either.

Finally, given how the lifecycle works, removing prepublish as an event would require special conditional logic and would violate the symmetry of the model, which would lead to yet more confusion and complaints (and the attendant support load that comes with that), so we're not going to do that.

Contributor

othiym23 commented Oct 23, 2015

is there any chance that a tiny 1.x

No. The book is closed on npm@1. There is going to be one more release of it that warns people to upgrade to at least npm@2 and that early errors when users try to install scoped packages, strictly so that we can transition to npm@2 in Node 0.10 LTS.

and 2.x

Probably, although we'd be more likely to cherry-pick 1-3 onto the 2.x branch, or add a simple deprecation warning to uses of prepublish that its behavior is changing. That's an LTS discussion, and so will probably involve @zkat and the Node LTS working group.

The reason to wait a year between steps 3 and 4 is twofold - to give developers a chance to migrate to prepare and prepublishOnly, and to give everyone a chance to upgrade to newish versions of npm. There's no reason for people to stay on old versions of npm, even if for whatever reason they can't move off of old versions of Node. We've put a considerable amount of work into keeping npm backwards compatible all the way to Node 0.8. This is especially true for publishers, who need to keep reasonably up to date just so they have an npm that works with the current registry architecture.

I don't think it's especially onerous to include "in order to publish this package, you must be running at least npm@3.4.5" in CONTRIBUTING.md. If you do want to go the extra mile and support old versions of npm, having a dependency on in-publish doesn't seem terrible, either.

Finally, given how the lifecycle works, removing prepublish as an event would require special conditional logic and would violate the symmetry of the model, which would lead to yet more confusion and complaints (and the attendant support load that comes with that), so we're not going to do that.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Oct 23, 2015

Contributor

Thanks for the detailed answer! If steps 1-3 make it onto 2.x, then I am content to not care about npm 1.

Sadly, CONTRIBUTING.md doesn't help me because documentation is advisory, not enforcement.

I don't understand the last paragraph, but I trust that you know the details far better than I :-)

Contributor

ljharb commented Oct 23, 2015

Thanks for the detailed answer! If steps 1-3 make it onto 2.x, then I am content to not care about npm 1.

Sadly, CONTRIBUTING.md doesn't help me because documentation is advisory, not enforcement.

I don't understand the last paragraph, but I trust that you know the details far better than I :-)

@othiym23

This comment has been minimized.

Show comment
Hide comment
@othiym23

othiym23 Oct 23, 2015

Contributor

Sadly, CONTRIBUTING.md doesn't help me because documentation is advisory, not enforcement.

To get a little more opinionated for a moment, if you can't get across to your collaborators the basic requirements for working with the code base, there are organizational problems that are far graver than the confusion arising from using inconsistent tool versions.

Contributor

othiym23 commented Oct 23, 2015

Sadly, CONTRIBUTING.md doesn't help me because documentation is advisory, not enforcement.

To get a little more opinionated for a moment, if you can't get across to your collaborators the basic requirements for working with the code base, there are organizational problems that are far graver than the confusion arising from using inconsistent tool versions.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Oct 23, 2015

Contributor

That's entirely true - but one could make the same argument about style, and yet using a linter to guarantee consistency is so common a practice that it's often considered bad practice to not do so. People are people, and checks that aren't automated by software will be occasionally skipped.

Contributor

ljharb commented Oct 23, 2015

That's entirely true - but one could make the same argument about style, and yet using a linter to guarantee consistency is so common a practice that it's often considered bad practice to not do so. People are people, and checks that aren't automated by software will be occasionally skipped.

kentcdodds added a commit to kentcdodds/api-check that referenced this issue Oct 26, 2015

@shellscape

This comment has been minimized.

Show comment
Hide comment
@shellscape

shellscape Oct 27, 2015

👍 to the proposal

👍 to the proposal

@toxicFork

This comment has been minimized.

Show comment
Hide comment
@toxicFork

toxicFork Nov 18, 2015

👍 it's really hurting me in many projects

👍 it's really hurting me in many projects

toxicFork added a commit to toxicFork/react-three-renderer that referenced this issue Nov 18, 2015

@peterjuras

This comment has been minimized.

Show comment
Hide comment
@peterjuras

peterjuras Nov 28, 2015

+1

Just saw it by accident, makes no sense to me at all. Now I have to run prepublish work manually to prevent the script from being run when another user installs my modules.

+1

Just saw it by accident, makes no sense to me at all. Now I have to run prepublish work manually to prevent the script from being run when another user installs my modules.

sean-clayton pushed a commit to sean-clayton/react-stackoverflow-profile that referenced this issue Dec 2, 2015

@silkentrance

This comment has been minimized.

Show comment
Hide comment
@silkentrance

silkentrance Dec 5, 2015

How about extending npm update to also install missing dependencies instead of using npm install?

Having that, you can keep your pre-publish behavior and still be backwards compatible without having the whole lot of prepublishOnly, deprecate, prepare and whatnot.

How about extending npm update to also install missing dependencies instead of using npm install?

Having that, you can keep your pre-publish behavior and still be backwards compatible without having the whole lot of prepublishOnly, deprecate, prepare and whatnot.

@othiym23

This comment has been minimized.

Show comment
Hide comment
@othiym23

othiym23 Dec 5, 2015

Contributor

That would be overloading the semantics of an existing command to work around the edge cases of another command, and would incur more complexity and cognitive load than the current solution, which has a few moving parts, but is simple, and leads to a natural deprecation path over time.

Contributor

othiym23 commented Dec 5, 2015

That would be overloading the semantics of an existing command to work around the edge cases of another command, and would incur more complexity and cognitive load than the current solution, which has a few moving parts, but is simple, and leads to a natural deprecation path over time.

@silkentrance

This comment has been minimized.

Show comment
Hide comment
@silkentrance

silkentrance Dec 5, 2015

@othiym23 talking about semantics

why not just distinguish between

package/package.json
package/index.js

> npm install package

and

> cd package
> npm install

while the aforementioned would try to publish the package to the local npm package cache, including also running the prepublish hook, the latter would simply skip the prepublish hook and just install both the dev dependencies and the runtime dependencies.

while the latter, i.e. installing both dev dependencies and runtime dependencies is also pretty much overloaded in a production environment where one does not need any dev dependencies, the overall approach would make things even easier for you, or would it not?

@othiym23 talking about semantics

why not just distinguish between

package/package.json
package/index.js

> npm install package

and

> cd package
> npm install

while the aforementioned would try to publish the package to the local npm package cache, including also running the prepublish hook, the latter would simply skip the prepublish hook and just install both the dev dependencies and the runtime dependencies.

while the latter, i.e. installing both dev dependencies and runtime dependencies is also pretty much overloaded in a production environment where one does not need any dev dependencies, the overall approach would make things even easier for you, or would it not?

@JSteunou

This comment has been minimized.

Show comment
Hide comment
@JSteunou

JSteunou Dec 15, 2015

Oh my +1 on this one, flat deps & prepublish fixed was the 2 things I need sooo much

Oh my +1 on this one, flat deps & prepublish fixed was the 2 things I need sooo much

@JSteunou

This comment has been minimized.

Show comment
Hide comment
@JSteunou

JSteunou Dec 16, 2015

Maybe everyone already knows about this nice go around but I did not so I link it here, maybe it could help https://github.com/iarna/in-publish

Maybe everyone already knows about this nice go around but I did not so I link it here, maybe it could help https://github.com/iarna/in-publish

@ryanve ryanve referenced this issue in airbnb/enzyme Jun 27, 2017

Closed

Support npm@5 scripts behavior #1014

@cfnelson cfnelson referenced this issue in okgrow/merge-graphql-schemas Jul 7, 2017

Closed

fix: babel transform-runtime bug #69

elliottsj added a commit to elliottsj/babel-plugin-inline-import that referenced this issue Jul 14, 2017

OliverJAsh added a commit to OliverJAsh/io-ts-reporters that referenced this issue Jul 14, 2017

OliverJAsh added a commit to OliverJAsh/decode-ts that referenced this issue Jul 14, 2017

OliverJAsh added a commit to OliverJAsh/twitter-api-ts that referenced this issue Jul 14, 2017

OliverJAsh added a commit to OliverJAsh/fp-express-validation that referenced this issue Jul 16, 2017

OliverJAsh added a commit to OliverJAsh/fp-express-validation that referenced this issue Jul 16, 2017

anko added a commit to anko/eslisp that referenced this issue Jul 30, 2017

Use prepare npm script instead of prepublish
In response to an npm deprecation notice I got when publishing, included
here for posterity:

    npm WARN prepublish-on-install As of npm@5, `prepublish` scripts are deprecated.
    npm WARN prepublish-on-install Use `prepare` for build steps and `prepublishOnly` for upload-only.
    npm WARN prepublish-on-install See the deprecation note in `npm help scripts` for more information.

This is apparently to address how confusing the script's name was in
certain contexts, as discussed in
npm/npm#10074.

@JReinhold JReinhold referenced this issue in alabeduarte/feedparser-promised Aug 4, 2017

Merged

Transpile sources to ES5 #16

jbergstroem added a commit to jbergstroem/choo that referenced this issue Oct 3, 2017

package: `prepublish` -> `prepare`
`prepublish` is deprecated in favor of `prepare` and `prepublishOnly`.
Seeing how we want `npm run build` invoked only prior to publishing,
opt for `prepublishOnly`.

Refs: npm/npm#10074

jbergstroem added a commit to jbergstroem/choo that referenced this issue Oct 3, 2017

package: `prepublish` -> `prepublishOnly`
`prepublish` is deprecated in favor of `prepare` and `prepublishOnly`.
Seeing how we want `npm run build` invoked only prior to publishing,
opt for `prepublishOnly`.

Refs: npm/npm#10074

yoshuawuyts added a commit to choojs/choo that referenced this issue Oct 3, 2017

package: `prepublish` -> `prepublishOnly` (#573)
`prepublish` is deprecated in favor of `prepare` and `prepublishOnly`.
Seeing how we want `npm run build` invoked only prior to publishing,
opt for `prepublishOnly`.

Refs: npm/npm#10074

FabienMotte added a commit to fm-ph/quark-crypto that referenced this issue Oct 18, 2017

FabienMotte added a commit to fm-ph/quark-log that referenced this issue Oct 18, 2017

FabienMotte added a commit to fm-ph/quark-utils that referenced this issue Oct 18, 2017

FabienMotte added a commit to fm-ph/quark-state that referenced this issue Oct 18, 2017

FabienMotte added a commit to fm-ph/quark-signal that referenced this issue Oct 18, 2017

FabienMotte added a commit to fm-ph/quark-decorators that referenced this issue Oct 18, 2017

FabienMotte added a commit to fm-ph/quark-package-starter that referenced this issue Oct 18, 2017

@cag cag referenced this issue in gnosis/pm-contracts Oct 20, 2017

Merged

Use prepublishOnly #59

@edm00se edm00se referenced this issue in edm00se/urls Nov 20, 2017

Closed

NPM Publish Issue #7

edm00se added a commit to edm00se/urls that referenced this issue Nov 20, 2017

fix(npm): npm prepare instead of prepublish
npm deprecated prepublish, ref: npm/npm#10074

closes #7

thisKai added a commit to thisKai/tilt-effect that referenced this issue Dec 1, 2017

ahmader added a commit to ahmader/node-zoho that referenced this issue Feb 8, 2018

Updating Dependencies
- Update README badges
- Update dependencies and devDependencies
- Rename npm script prepublish to prepare as npm/npm#10074
- Add npm-shrinkwrap.json in bump.
- Update Integration options
- Update Grunt bump script

@ahmader ahmader referenced this issue in ahmader/node-zoho Feb 8, 2018

Merged

Updating Dependencies #29

@jdesboeufs jdesboeufs referenced this issue in etalab/codes-postaux Feb 27, 2018

Merged

Simplification du packaging Node.js #21

mroderick added a commit to sinonjs/referee that referenced this issue Mar 3, 2018

Fix broken build
* mkdirp dependency missing
* don't run build during prepublish, as that's run by install in node 4

npm/npm#10074

@insin insin referenced this issue in insin/nwb Mar 17, 2018

Merged

add prepare scripts #439

@TheSharpieOne TheSharpieOne referenced this issue in FezVrasta/react-popper Mar 23, 2018

Merged

build: refactored build process #109

2 of 2 tasks complete

elboman added a commit to elboman/gatsby-remark-embedded-codesandbox that referenced this issue Apr 24, 2018

EnTeQuAk added a commit to mozilla/addons-linter that referenced this issue Jun 25, 2018

Rename 'prepublish' to 'prepublishOnly'.
This got deprecated in npm 6, see
npm/npm#10074 for much much more details.

@EnTeQuAk EnTeQuAk referenced this issue in mozilla/addons-linter Jun 25, 2018

Merged

Rename 'prepublish' to 'prepublishOnly'. #2070

@0xR 0xR referenced this issue in yarax/swagger-to-graphql Jun 30, 2018

Merged

Fix package json #85

@gabascc

This comment has been minimized.

Show comment
Hide comment
@gabascc

gabascc Jul 8, 2018

MIT License

Copyright (c) [year] [fullname]

Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
in the Software without restriction, including without limitation the rights
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
copies of the Software, and to permit persons to whom the Software is
furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all
copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE.

gabascc commented Jul 8, 2018

MIT License

Copyright (c) [year] [fullname]

Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
in the Software without restriction, including without limitation the rights
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
copies of the Software, and to permit persons to whom the Software is
furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all
copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.