From e6bb138bd2295ee4b96e93405abffdc101ffed1b Mon Sep 17 00:00:00 2001 From: Dan Duvall Date: Mon, 17 Sep 2018 15:41:48 -0700 Subject: Install `node_modules` to application directory The original design decision to install `node_modules` to a central location outside the application directory was for a purported development use case not yet realized. It has led to confusion and unintended failures, and is just generally weird. Let's right this wrong before it fossilizes into debt, like a hysterical raisin in the sun. Bug: T204591 Change-Id: I2e4797833471596b10b6b4a2705055050ca98ac6 --- config/node.go | 31 ++++++++++++------------------- config/node_test.go | 18 +++++------------- 2 files changed, 17 insertions(+), 32 deletions(-) diff --git a/config/node.go b/config/node.go index 438e715..a6a22ea 100644 --- a/config/node.go +++ b/config/node.go @@ -2,7 +2,6 @@ package config import ( "gerrit.wikimedia.org/r/blubber/build" - "path" ) // NodeConfig holds configuration fields related to the Node environment and @@ -27,53 +26,47 @@ func (nc *NodeConfig) Merge(nc2 NodeConfig) { } // InstructionsForPhase injects instructions into the build related to Node -// dependency installation and setting of the NODE_ENV, NODE_PATH, and PATH -// environment variables. +// dependency installation and setting of the NODE_ENV. // // PhasePreInstall // // Installs Node package dependencies declared in requirements files into the -// shared library directory (/opt/lib). Only production related packages are -// install if NodeConfig.Env is set to "production" in which case `npm dedupe` -// is also run. Installing dependencies during the build.PhasePreInstall phase -// allows a compiler implementation (e.g. Docker) to produce cache-efficient -// output so only changes to package.json will invalidate these steps of the -// image build. +// application directory. Only production related packages are install if +// NodeConfig.Env is set to "production" in which case `npm dedupe` is also +// run. Installing dependencies during the build.PhasePreInstall phase allows +// a compiler implementation (e.g. Docker) to produce cache-efficient output +// so only changes to package.json will invalidate these steps of the image +// build. // // PhasePostInstall // -// Injects build.Env instructions for NODE_ENV, NODE_PATH, and PATH, setting -// the environment according to the configuration, ensuring that packages -// installed during build.PhasePreInstall are found by Node, and that any -// installed binaries are found by shells. +// Injects build.Env instructions for NODE_ENV, setting the environment +// according to the configuration. // func (nc NodeConfig) InstructionsForPhase(phase build.Phase) []build.Instruction { switch phase { case build.PhasePreInstall: if len(nc.Requirements) > 0 { npmInstall := build.RunAll{[]build.Run{ - {"cd", []string{LocalLibPrefix}}, {"npm install", []string{}}, }} if nc.Env == "production" { - npmInstall.Runs[1].Arguments = []string{"--production"} + npmInstall.Runs[0].Arguments = []string{"--production"} npmInstall.Runs = append(npmInstall.Runs, build.Run{"npm dedupe", []string{}}, ) } return append( - build.SyncFiles(nc.Requirements, LocalLibPrefix), + build.SyncFiles(nc.Requirements, "."), npmInstall, ) } case build.PhasePostInstall: if nc.Env != "" || len(nc.Requirements) > 0 { return []build.Instruction{build.Env{map[string]string{ - "NODE_ENV": nc.Env, - "NODE_PATH": path.Join(LocalLibPrefix, "node_modules"), - "PATH": path.Join(LocalLibPrefix, "node_modules", ".bin") + ":${PATH}", + "NODE_ENV": nc.Env, }}} } } diff --git a/config/node_test.go b/config/node_test.go index 0cb48fa..ef29ee5 100644 --- a/config/node_test.go +++ b/config/node_test.go @@ -69,9 +69,8 @@ func TestNodeConfigInstructionsNonProduction(t *testing.T) { t.Run("PhasePreInstall", func(t *testing.T) { assert.Equal(t, []build.Instruction{ - build.Copy{[]string{"package.json"}, "/opt/lib/"}, + build.Copy{[]string{"package.json"}, "./"}, build.RunAll{[]build.Run{ - {"cd", []string{"/opt/lib"}}, {"npm install", []string{}}, }}, }, @@ -83,9 +82,7 @@ func TestNodeConfigInstructionsNonProduction(t *testing.T) { assert.Equal(t, []build.Instruction{ build.Env{map[string]string{ - "NODE_ENV": "foo", - "NODE_PATH": "/opt/lib/node_modules", - "PATH": "/opt/lib/node_modules/.bin:${PATH}", + "NODE_ENV": "foo", }}, }, cfg.InstructionsForPhase(build.PhasePostInstall), @@ -107,9 +104,8 @@ func TestNodeConfigInstructionsProduction(t *testing.T) { t.Run("PhasePreInstall", func(t *testing.T) { assert.Equal(t, []build.Instruction{ - build.Copy{[]string{"package.json", "package-lock.json"}, "/opt/lib/"}, + build.Copy{[]string{"package.json", "package-lock.json"}, "./"}, build.RunAll{[]build.Run{ - {"cd", []string{"/opt/lib"}}, {"npm install", []string{"--production"}}, {"npm dedupe", []string{}}, }}, @@ -122,9 +118,7 @@ func TestNodeConfigInstructionsProduction(t *testing.T) { assert.Equal(t, []build.Instruction{ build.Env{map[string]string{ - "NODE_ENV": "production", - "NODE_PATH": "/opt/lib/node_modules", - "PATH": "/opt/lib/node_modules/.bin:${PATH}", + "NODE_ENV": "production", }}, }, cfg.InstructionsForPhase(build.PhasePostInstall), @@ -151,9 +145,7 @@ func TestNodeConfigInstructionsEnvironmentOnly(t *testing.T) { assert.Equal(t, []build.Instruction{ build.Env{map[string]string{ - "NODE_ENV": "production", - "NODE_PATH": "/opt/lib/node_modules", - "PATH": "/opt/lib/node_modules/.bin:${PATH}", + "NODE_ENV": "production", }}, }, cfg.InstructionsForPhase(build.PhasePostInstall), -- cgit v1.2.1