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

Compatibility with yarn #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

igorklopov
Copy link

@igorklopov igorklopov commented Dec 9, 2016

yarn === npm install also yarn !== npm publish

@iarna
Copy link
Owner

iarna commented Dec 17, 2016

Huh, I was under the impression that Yarn was using npm to run lifecycle scripts? Is that just for an npm run stand in?

@igorklopov
Copy link
Author

igorklopov commented Dec 17, 2016

Seems they don't use npm code, but they say "yay compatibility":
https://github.com/yarnpkg/yarn/blob/master/src/cli/commands/install.js#L869

@iarna
Copy link
Owner

iarna commented Dec 17, 2016

Kind of, they don't support prepare or prepublishOnly, which are in npm today and were specced out from around the time they started work on yarn (maybe before). =p

@FezVrasta
Copy link

May you merge this please?

@igorklopov
Copy link
Author

igorklopov commented Jan 2, 2017

Please don't be demanding to yarn. We ask you to consider yarn as npm@3. Once yarn supports prepare and prepublishOnly i promise to make a new PR probing yarn version and deciding to turn on this discussed PR feature or not.

@iarna
Copy link
Owner

iarna commented Jan 2, 2017

Ok, so the main delay here was that I wanted to verify that install was the only time Yarn would run prepublish. It is not.

Yarn has a publish command that runs the prepublish lifecycle:

See: lib/cli/commands/publish.js in the Yarn source.

Because of that, this PR is going to need revision to detect that usage.

@garthk
Copy link

garthk commented Jan 4, 2017

Treating prepublish as equivalent to publish will also help lerna users using transpilers. My situation:

  • lerna bootstrap runs npm prepublish in each package directory
  • in-publish fails, so
  • Transpiling doesn't happen, so
  • dist/index.js doesn't exist, so
  • Attempts to require the package fail

This private fork does the trick:

'use strict'
function inCommand (cmd) {
  try {
    var npm_config_argv = JSON.parse(process.env['npm_config_argv'])
  } catch (e) {
    return false
  }

  if (typeof npm_config_argv !== 'object') process.exit(1)
  if (!npm_config_argv.cooked) process.exit(1)
  if (!npm_config_argv.cooked instanceof Array) process.exit(1)

  var V
  if (npm_config_argv.cooked[0] === 'run') npm_config_argv.cooked.shift()
  while ((V = npm_config_argv.cooked.shift()) !== undefined) {
    if (/^-/.test(V)) continue
    if (cmd.test(V)) return true
    return false
  }
  return false
}

exports.inPublish = function () {
  return inCommand(/^(?:pre)?pu(b(l(i(sh?)?)?)?)?$/)
}

exports.inInstall = function () {
  return inCommand(/^(?:pre)?i(n(s(t(a(ll?)?)?)?)?)?$/)
}

I'd argue it's a breaking change, and would require a semver major bump.

@iarna
Copy link
Owner

iarna commented Jan 4, 2017

@garthk It's not clear to me what you're trying to change? Can you PR that so I can see a diff?

garthk added a commit to garthk/in-publish that referenced this pull request Jan 5, 2017
Diff as requested in discussion on iarna#10. Not sure it'll work for all use cases, but it works for mine, so I've quietly forked it until `npm@4` comes along and straightens this mess out for everyone.
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.

4 participants