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

Add pre and post install script calls #800

Merged
merged 2 commits into from Oct 12, 2016

Conversation

FLGMwt
Copy link
Contributor

@FLGMwt FLGMwt commented Oct 12, 2016

Summary
Adds calls to the following named scripts: preinstall, install, postinstall and wraps prepublish in an if !prod, as it is in npm install Should resolve #783 Happy to discuss.

Test plan
Given the following package.json:

{
  "name": "junk",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "scripts": {
    "preinstall": "echo 'preinstall'",
    "install": "echo 'install'",
    "postinstall": "echo 'postinstall'",
    "prepublish": "echo 'prepublish'"
  }
}

npm install outputs the following:

PS C:\git\junk> npm i

> junk@1.0.0 preinstall C:\git\junk
> echo 'preinstall'

'preinstall'

> junk@1.0.0 install C:\git\junk
> echo 'install'

'install'

> junk@1.0.0 postinstall C:\git\junk
> echo 'postinstall'

'postinstall'

> junk@1.0.0 prepublish C:\git\junk
> echo 'prepublish'

'prepublish'
npm WARN junk@1.0.0 No description
npm WARN junk@1.0.0 No repository field.

yarn, however only executes prepublish (before changes):

PS C:\git\junk> yarn
yarn install v0.15.1
info No lockfile found.
success Nothing to install.
$ echo 'prepublish'
'prepublish'
Done in 0.06s.

After changes:

PS C:\git\junk> yarn
yarn install v0.15.1
info No lockfile found.
$ echo 'preinstall'
'preinstall'
success Nothing to install.
$ echo 'install'
'install'
$ echo 'postinstall'
'postinstall'
$ echo 'prepublish'
'prepublish'
Done in 0.09s.

and with --prod flag:

PS C:\git\junk> yarn --prod
yarn install v0.15.1
info No lockfile found.
$ echo 'preinstall'
'preinstall'
success Nothing to install.
$ echo 'install'
'install'
$ echo 'postinstall'
'postinstall'
Done in 0.08s.

I'm also working through how to add unit tests for this and can hold merge for that is desired.

@FLGMwt
Copy link
Contributor Author

FLGMwt commented Oct 12, 2016

Also #721 which might be a dupe.

@Jessidhia
Copy link

Note that npm means to deprecate how prepublish currently works in npm 4.0, to be released this month: npm/npm#10074

@beheh beheh mentioned this pull request Oct 12, 2016
@FLGMwt
Copy link
Contributor Author

FLGMwt commented Oct 12, 2016

@Kovensky Nice catch!

After reviewing conversation there, it looks like it's still pretty up in the air what npm plans to do exactly to change the behavior. I'd be up for porting whatever they implement if that's desired, but these changes at least get us closer to npm compatibility (and resolves issues for a few people migrating).

That said, I'm happy to remove prepublish and just get the install hooks in. Looking for guidance : )

@jamesjnadeau
Copy link

@FLGMwt awesome work on this!

IMHO, not having these hooks is a show stopper to a lot of projects using yarn.

I would say feature parity with npm is what you should be shooting for, and having prepublish work the same way is probably best.

@ljharb
Copy link

ljharb commented Oct 12, 2016

Unless every single lifecycle script functions exactly the same as it does on npm, then yarn is not a drop-in replacement for npm - which I understand to be the goal. That includes prepublish's awful behavior.

@sebmck sebmck merged commit cb00d27 into yarnpkg:master Oct 12, 2016
@billxinli
Copy link

@kittens Curious on when this fix will get pushed to a release. This fix is going to fix a lot of issues with modules that depends on these hooks

@Jessidhia
Copy link

Jessidhia commented Oct 13, 2016

Just a note: npm@2 will run preinstall scripts before installing dependencies, but npm@3 runs preinstall only after all dependencies are installed.

This patch runs preinstall before installing dependencies, so preinstall scripts can't call anything but pre-existing system commands. cross-env, for example, is out.

I noticed it because I (ab)used this behavior to prevent accidental npm installs with the global npm@2 instead of the npm@3 required in our nodenv setup.

@dominictarr
Copy link

@ljharb A beer says that prepublish isn't that big a problem, and this is a good way to find out.

@ljharb
Copy link

ljharb commented Oct 16, 2016

@dominictarr if you're right, I will buy you a whole case. But if there are non-zero packages that depends on it, that would be enough for it to be a huge problem.

@dominictarr
Copy link

in the browser world, not everything is completely compatible, and, well, it's annoying some times, but we survive. I wasn't around at the time when javascript was brand new... but I'm gonna hazard to suggest that "mutating" a little bit, could actually make something better.

And in any case, there isn't a spec for npm just an implementation, however if there are more than one implementation, maybe it's time to start thinking about a spec. (maybe wait another 15 minutes though)

@lipis
Copy link

lipis commented Oct 17, 2016

@dominictarr on point.. since there are no specs let's rethink some of the procedures and not blindly copy stuff from npm.. just because.

@ljharb
Copy link

ljharb commented Oct 17, 2016

All the things browsers agree upon are the spec - that's true in the JS standard as well as in browser standards. Since npm has been the only implementation for this long, what it does is the spec. If yarn deviates, yarn will lose out.

@curtisblackwell
Copy link

Any idea when we can expect the next release so we can rely on this functionality?

@jordaaash
Copy link

Any update on the status of a release including this?

@janusch
Copy link

janusch commented Jan 9, 2017

I am wondering if this also works for the 'yarn-offline-mirror' feature?
E.g. in node-sass the postinstall will download the correct binaries, are they added to the offline-mirror?

Some clarification would be very helpful to me!

@lordgreg
Copy link

lordgreg commented Feb 7, 2017

Yarn doesn't install selenium webdriver when installing node module protractor. Using npm install protractor works as it should.

@ndbroadbent
Copy link

ndbroadbent commented Feb 17, 2017

Hello, I am testing the yarn nightly build (v0.22.0-20170215.1538), and this doesn't seem to be working for me. Or maybe I just don't understand what it should be doing.

I have forked a package and am trying to install it from GitHub:

yarn add ndbroadbent/redux-persist-migrate

This package.json contains a prepublish line:

"prepublish": "npm run clean && npm run build"

This is not being run during the installation. I would expect this to be run, since GitHub is only for source code, and people don't usually check in the build artifacts.

I just read through this issue on npm, and it looks like npm also doesn't run the prepublish script. I think npm should run this script when a package is installed from github, and I think yarn should, too.

Or should I just add this step as a preinstall hook on my own fork? Or just run the preinstall step myself and check in the built files?

EDIT: Ah, sorry, I just realized that this is for the prepublish hooks in your own package.json, and not in the dependencies you are installing. Even so, I would appreciate any advice!

@jamesjnadeau
Copy link

@ndbroadbent: is preinstall what you are looking for?

@ndbroadbent
Copy link

ndbroadbent commented Feb 17, 2017

@jamesjnadeau I tried adding a preinstall hook, but that doesn't seem to work either. yarn is not even running the prepublish or preinstall hooks when installing from a local directory. When I run: npm i ../redux-persist-migrate, I see it runs the prepublish and preinstall hooks.

However, npm i ndbroadbent/redux-persist-migrate doesn't run those hooks, and neither does yarn add ndbroadbent/redux-persist-migrate. (Installing from GitHub)

So I guess the only way is to push a new branch with the build artifacts checked in?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong script runed after install