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

Don't require babel to be installed globally #409

Closed
wants to merge 1 commit into from
Closed

Don't require babel to be installed globally #409

wants to merge 1 commit into from

Conversation

wincent
Copy link
Contributor

@wincent wincent commented Oct 1, 2015

While investigating an issue reported for the todomvc example
(#408), I tried to repro as follows:

git checkout master
git fetch upstream
git merge upstream/master
npm install && npm run update-schema && npm run start

But it went boom during the npm install because I didn't have babel
installed globally:

sh: babel: command not found

(Full output at https://gist.github.com/wincent/3221694ee3fdc46084f4)

What's happening here:

  • example's preinstall tries to run install from the top-level
  • top-level install tries to install babel-relay-plugin dependency
  • babel-relay-plugin build step runs and tries to shell out to babel
  • babel call will blow up if not globally installed, even though babel
    is listed as a dev dependency

Workaround:

npm install -g babel

Fix employed here: we add npm install --ignore-scripts to the
babel-relay-plugin build recipe, causing it to become available in
node_modules and therefore the $PATH when the babel command later
runs. Note that without the --ignore-scripts we get into an infinite
loop, although I am not sure of the exact chain of cause and effect
because all we see on the console at that point is this, endlessly
repeated:

> babel-relay-plugin@0.2.6 build /Users/glh/code/relay/scripts/babel-relay-plugin
> rm -rf lib; npm install; npm run babel

Also note the use of ; instead of && in the interest of preserving
potential future compatibility with Windows.

I think this change should be ok for end users or the
babel-relay-plugin, as they don't end up running the build script and
instead just get whatever we built into lib/.

Test Plan: Blow away global babel (npm uninstall -g babel) and example
node_modules and then npm install from inside example or top-level,
see it no longer blows from.

@wincent
Copy link
Contributor Author

wincent commented Oct 1, 2015

CC @yungsters who knows this stuff; I might be unaware of some constraints.

@yungsters
Copy link
Contributor

Note that without the --ignore-scripts we get into an infinite loop, although I am not sure of the exact chain of cause and effect [...]

According to npm-build(1), the build script is a plumbing command called by npm install and npm link. So if the build script calls npm install, you have an infinite loop.


My understanding is that if you have a file://... dependency in your package.json (like our examples do), it won't install the dependency's dependencies. This is why babel-relay-plugin's dependency on babel is not installed, which is what causes the problem raised by this pull request.

If you take a look at the example's preinstall script, you'll notice it works around this by installing the root directory's dependencies:

"preinstall": "cd ../.. && npm install --ignore-scripts",

The --ignore-scripts flag means npm install should install the dependencies without running the build script. Why's that? Well, it appears that running npm install does run the build script of file://... dependencies. If we left out --ignore-scripts, we would end up building the react-relay package twice (which is not wrong, just unnecessarily time consuming).

NOTE:
In my opinion, npm's behavior around file://... dependencies is kind of messed up.


So back to the problem at hand — babel is a dependency of babel-relay-plugin, which is a file://... dependency of the examples. The reason that babel has an error when installing an example is that the babel-relay-plugin's build script is being executed without first installing its dependencies.

I don't think that changing babel-relay-plugin's build script is the right fix. This would be the equivalent of replacing the example's existing preinstall script by adding npm install --ignore-scripts to react-relay's build script, which is definitely wrong.

Instead, it seems that any package that has a file://... dependency needs to ensure that dependency's package dependencies are installed using npm install --ignore-scripts. So instead, we can just change the example's preinstall script to also correctly handle babel-relay-plugin:

"preinstall": "cd ../.. && npm install --ignore-scripts && cd scripts/babel-relay-plugin && npm install --ignore-scripts",

@wincent wincent closed this Oct 13, 2015
@wincent wincent deleted the no-global branch October 13, 2015 14:17
@wincent
Copy link
Contributor Author

wincent commented Oct 13, 2015

Whoops, I accidentally closed this. I'll fix it up and resubmit.

@wincent wincent restored the no-global branch October 13, 2015 15:59
@wincent wincent reopened this Oct 13, 2015
While investigating an issue reported for the todomvc example
(#408), I found that `npm
install` in an example directory was blowing up because I didn't have
`babel` installed globally:

```
sh: babel: command not found
```

(Full output at https://gist.github.com/wincent/3221694ee3fdc46084f4)

What's happening here:

- Example's `preinstall` runs `install` from the top-level.
- Top-level `install` installs its babel-relay-plugin dependency from
  NPM.
- Example's `install` runs, and runs `build` from the babel-relay-plugin
  `file:../` dependency.
- Note that `install` hasn't happened inside the `file:../` dependency
  yet, which means that the `babel` call will blow up if not globally
  installed.

Fix:

- Make sure the example's `preinstall` step `cd`'s into the `file:../`
  dependency and installs its dependencies (ie. `babel`) up front.

Known issue:

- `&&` won't work on Windows. I could "fix" that by using `;` instead,
  but the better fix is to turn these into cross-platform node scripts.
  I'll create a separate issue for that, if I can't find an existing
  one.

Tested: Blow away global babel (`npm uninstall -g babel`), example
`node_modules` and top-level `node_modules`, then `npm install` from
inside an example or top-level; see it no longer blows up. Also `npm run
update-schema` works, as does `npm run start`.
@wincent
Copy link
Contributor Author

wincent commented Oct 13, 2015

Ok, I think this should be good to go now.

@facebook-github-bot import

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/1650880361858182/int_phab to review.

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

Successfully merging this pull request may close these issues.

3 participants