[WIP] language/node: fix prepublish scripts

Library/Homebrew/language/node.rb

@@ -1,10 +1,44 @@
+require "json"
+
 module Language
   module Node
     def self.npm_cache_config
       "cache=#{HOMEBREW_CACHE}/npm_cache"
     end
 
-    def self.pack_for_installation
+    def self.node_system(cmd, *args)
+      ohai "#{cmd} #{args * " "}"
+      if ARGV.verbose?
+        safe_system(cmd, *args)
+      else
+        quiet_system(cmd, *args)
+        raise ErrorDuringExecution.new(cmd, args) unless $CHILD_STATUS.exitstatus.zero?
+      end
+    end
+
+    # Read https://gist.github.com/chrmoritz/34e4c4d7779d72b549e2fc41f77c365c
+    # for a complete overview of the edge cases this method has to handle.
+    def self.pack_for_installation(prepare_required: false)
+      # Rewrites the package.json so that npm pack will bundle all deps
+      # into a self-contained package, so that we avoid installing them a
+      # second time during the final installation of the package to libexec.
+      pkg_json = JSON.parse(IO.read("package.json"))
+      if pkg_json["dependencies"]
+        pkg_json["bundledDependencies"] = pkg_json["dependencies"].keys
+        IO.write("package.json", JSON.pretty_generate(pkg_json))
+      end
+
+      # If `prepare_required` is false we pass `--production` to install all
+      # production deps (no devDeps) for bundling them into the pack and also
+      # to prevent the toplevel prepare / prepublish script from being executed
+      # while still executing all lifecycle scripts for the deps.
+      # Otherwise we omit `--production` to install all deps (devDeps might be
+      # required in `prepare`). This already executes the prepare script too,
+      # so that we can continue to `npm pack` with `--ignore-scripts`.
+      install_args = local_npm_install_args
+      install_args << "--production" unless prepare_required
+      node_system "npm", "install", *install_args
+
       # Homebrew assumes the buildpath/testpath will always be disposable
       # and from npm 5.0.0 the logic changed so that when a directory is
       # fed to `npm install` only symlinks are created linking back to that
@@ -30,13 +64,13 @@ def self.setup_npm_environment
       end
     end
 
-    def self.std_npm_install_args(libexec)
+    def self.std_npm_install_args(libexec, prepare_required: false)
       setup_npm_environment
       # tell npm to not install .brew_home by adding it to the .npmignore file
       # (or creating a new one if no .npmignore file already exists)
       open(".npmignore", "a") { |f| f.write("\n.brew_home\n") }
 
-      pack = pack_for_installation
+      pack = pack_for_installation(prepare_required: prepare_required)
 
       # npm install args for global style module format installed into libexec
       %W[

Library/Homebrew/test/language/node_spec.rb

@@ -1,4 +1,5 @@
 require "language/node"
+require "json"
 
 describe Language::Node do
   describe "#setup_npm_environment" do
@@ -25,23 +26,52 @@
     npm_install_arg = "libexec"
     npm_pack_cmd = "npm pack --ignore-scripts"
 
-    it "raises error with non zero exitstatus" do
+    it "raises error with non zero npm pack exitstatus" do
+      allow(IO).to receive(:read).and_return("{}")
+      allow(Language::Node).to receive(:node_system).with("npm", "install", "-ddd", "--build-from-source",
+        "--cache=#{HOMEBREW_CACHE}/npm_cache", "--production") { `true` }
       allow(Utils).to receive(:popen_read).with(npm_pack_cmd) { `false` }
       expect { subject.std_npm_install_args(npm_install_arg) }.to \
         raise_error("npm failed to pack #{Dir.pwd}")
     end
 
     it "raises error with empty npm pack output" do
-      allow(Utils).to receive(:popen_read).with(npm_pack_cmd) { `true` }
+      allow(IO).to receive(:read).and_return("{}")
+      allow(Language::Node).to receive(:node_system).with("npm", "install", "-ddd", "--build-from-source",
+        "--cache=#{HOMEBREW_CACHE}/npm_cache", "--production") { `true` }
+      allow(Utils).to receive(:popen_read).with(npm_pack_cmd) { `false` }
       expect { subject.std_npm_install_args(npm_install_arg) }.to \
         raise_error("npm failed to pack #{Dir.pwd}")
     end
 
-    it "does not raise error with a zero exitstatus" do
+    it "does not raise error with a zero npm pack exitstatus" do
+      allow(IO).to receive(:read).and_return("{}")
+      allow(Language::Node).to receive(:node_system).with("npm", "install", "-ddd", "--build-from-source",
+        "--cache=#{HOMEBREW_CACHE}/npm_cache", "--production") { `true` }
       allow(Utils).to receive(:popen_read).with(npm_pack_cmd) { `echo pack.tgz` }
       resp = subject.std_npm_install_args(npm_install_arg)
       expect(resp).to include("--prefix=#{npm_install_arg}", "#{Dir.pwd}/pack.tgz")
     end
+
+    it "does not raise error with prepare_required set" do
+      allow(IO).to receive(:read).and_return('{"name":"example","dependencies":{"foo":"1.2.3","bar":"4.5.6"}}')
+      expected_json = '{"name":"example","dependencies":{"foo":"1.2.3","bar":"4.5.6"},'\
+                      '"bundledDependencies":["foo","bar"]}'
+      expected_json = JSON.pretty_generate(JSON.parse(expected_json))
+      allow(IO).to receive(:write).with("package.json", expected_json) { `true` }
+      allow(Language::Node).to receive(:node_system)
+        .with("npm", "install", "-ddd", "--build-from-source", "--cache=#{HOMEBREW_CACHE}/npm_cache") { `true` }
+      allow(Utils).to receive(:popen_read).with("npm pack --ignore-scripts") { `echo pack.tgz` }
+      resp = subject.std_npm_install_args(npm_install_arg, prepare_required: true)
+      expect(resp).to include("--prefix=#{npm_install_arg}", "#{Dir.pwd}/pack.tgz")
+    end
+
+    it "raises error with non zero local npm install exitstatus" do
+      allow(IO).to receive(:read).and_return("{}")
+      allow(Language::Node).to receive(:node_system).and_raise("Failure while executing: npm install -ddd")
+      expect { subject.std_npm_install_args(npm_install_arg, prepare_required: true) }.to \
+        raise_error("Failure while executing: npm install -ddd")
+    end
   end
 
   specify "#local_npm_install_args" do

docs/Node-for-Formula-Authors.md

@@ -72,7 +72,15 @@ This will install your Node module in npm's global module style with a custom pr
 bin.install_symlink Dir["#{libexec}/bin/*"]
 ```
 
-**Note:** Because of a required workaround for `npm@5` calling `npm pack` we currently don't support installing modules (from non-npm registry tarballs), which require a prepublish step (e.g. for transpiling sources). See [Homebrew/brew#2820](https://github.com/Homebrew/brew/pull/2820) for more information.
+**Note:** If your Node module requires a prepare or prepublish step (e.g. for transpiling sources) it is recommended to use a npm registry tarball (which is already prepared/ prepublished) as the download url. If this is not possible, you have to pass `prepare_required: true` to `std_npm_install_args` like in:
+
+```ruby
+system "npm", "install", *Language::Node.std_npm_install_args(libexec, prepare_required: true)
+```
+
+This will run the `prepare` and `prepublish` scripts of your module before we pack the module and it's dependencies internally for installation.
+
+Not passing `prepare_required: true` will prevent the `prepare` and `prepublish` scripts of your top-level module from being executed, but all lifecycle scripts from it's dependencies are still executed as normal.
 
 ### Installing module dependencies locally with `local_npm_install_args`