Consitently use NODE_ENV and restructure npm package for clarity (#1304)

lib/webpacker.rb

@@ -19,6 +19,7 @@ def instance
 end
 
 require "webpacker/instance"
+require "webpacker/env"
 require "webpacker/configuration"
 require "webpacker/manifest"
 require "webpacker/compiler"

lib/webpacker/dev_server_runner.rb

@@ -14,7 +14,7 @@ def run
     private
       def load_config
         @config_file = File.join(@app_path, "config/webpacker.yml")
-        dev_server = YAML.load_file(@config_file)[ENV["RAILS_ENV"]]["dev_server"]
+        dev_server = YAML.load_file(@config_file)[ENV["NODE_ENV"]]["dev_server"]
 
         @hostname          = dev_server["host"]
         @port              = dev_server["port"]

lib/webpacker/env.rb

@@ -0,0 +1,39 @@
+class Webpacker::Env
+  DEFAULT = "production".freeze
+
+  delegate :config_path, :logger, to: :@webpacker
+
+  def self.inquire(webpacker)
+    new(webpacker).inquire
+  end
+
+  def initialize(webpacker)
+    @webpacker = webpacker
+  end
+
+  def inquire
+    fallback_env_warning unless current
+    (current || DEFAULT).inquiry
+  end
+
+  private
+    def current
+      (ENV["NODE_ENV"] || Rails.env).presence_in(available_environments)
+    end
+
+    def fallback_env_warning
+      logger.info "NODE_ENV=#{ENV["NODE_ENV"]} and RAILS_ENV=#{Rails.env} environment is not defined in config/webpacker.yml, falling back to #{DEFAULT} environment"
+    end
+
+    def available_environments
+      if config_path.exist?
+        YAML.load(config_path.read).keys
+      else
+        [].freeze
+      end
+    rescue Psych::SyntaxError => e
+      raise "YAML syntax error occurred while parsing #{config_path}. " \
+            "Please note that YAML must be consistently indented using spaces. Tabs are not allowed. " \
+            "Error: #{e.message}"
+    end
+end

lib/webpacker/instance.rb

@@ -8,9 +8,7 @@ def initialize(root_path: Rails.root, config_path: Rails.root.join("config/webpa
   end
 
   def env
-    (ENV["NODE_ENV"].presence_in(available_environments) ||
-      Rails.env.presence_in(available_environments) ||
-        "production".freeze).inquiry
+    @env ||= Webpacker::Env.inquire self
   end
 
   def config
@@ -32,13 +30,4 @@ def manifest
   def commands
     @commands ||= Webpacker::Commands.new self
   end
-
-  private
-    def available_environments
-      if config_path.exist?
-        YAML.load(config_path.read).keys
-      else
-        [].freeze
-      end
-    end
 end

package/__tests__/config.js

@@ -1,12 +1,12 @@
 /* global test expect, describe */
 
-const chdirApp = () => process.chdir('test/test_app')
-const chdirCwd = () => process.chdir(process.cwd())
-chdirApp()
+const { chdirTestApp, chdirCwd } = require('../utils/helpers')
+
+chdirTestApp()
 
 const config = require('../config')
 
-describe('Webpacker.yml config', () => {
+describe('Config', () => {
   afterAll(chdirCwd)
 
   test('should return extensions as listed in app config', () => {

package/__tests__/dev_server.js

@@ -0,0 +1,26 @@
+/* global test expect, describe */
+
+const { chdirTestApp, chdirCwd } = require('../utils/helpers')
+
+chdirTestApp()
+
+describe('DevServer', () => {
+  beforeEach(() => jest.resetModules())
+  afterAll(chdirCwd)
+
+  test('with NODE_ENV set to development', () => {
+    process.env.NODE_ENV = 'development'
+    process.env.WEBPACKER_DEV_SERVER_HOST = '0.0.0.0'
+    process.env.WEBPACKER_DEV_SERVER_PORT = 5000
+
+    const devServer = require('../dev_server')
+    expect(devServer).toBeDefined()
+    expect(devServer.host).toEqual('0.0.0.0')
+    expect(devServer.port).toEqual('5000')
+  })
+
+  test('with NODE_ENV set to production', () => {
+    process.env.NODE_ENV = 'production'
+    expect(require('../dev_server')).toEqual({})
+  })
+})

package/__tests__/env.js

@@ -0,0 +1,28 @@
+/* global test expect, describe */
+
+const { chdirTestApp, chdirCwd } = require('../utils/helpers')
+
+chdirTestApp()
+
+describe('Env', () => {
+  beforeEach(() => jest.resetModules())
+  afterAll(chdirCwd)
+
+  test('with NODE_ENV set to development', () => {
+    process.env.NODE_ENV = 'development'
+    expect(require('../env')).toEqual('development')
+  })
+
+  test('with undefined NODE_ENV and RAILS_ENV set to development', () => {
+    delete process.env.NODE_ENV
+    process.env.RAILS_ENV = 'development'
+    expect(require('../env')).toEqual('development')
+  })
+
+  test('with a non-standard environment', () => {
+    process.env.NODE_ENV = 'foo'
+    process.env.RAILS_ENV = 'foo'
+    expect(require('../env')).toEqual('production')
+    delete process.env.RAILS_ENV
+  })
+})

package/__tests__/index.js

@@ -0,0 +1,31 @@
+/* global test expect, describe */
+
+const { chdirTestApp, chdirCwd } = require('../utils/helpers')
+
+chdirTestApp()
+
+describe('Webpacker', () => {
+  beforeEach(() => jest.resetModules())
+  afterAll(chdirCwd)
+
+  test('with NODE_ENV set to development', () => {
+    process.env.NODE_ENV = 'development'
+    const { environment } = require('../index')
+    expect(environment.toWebpackConfig()).toMatchObject({
+      devServer: {
+        host: 'localhost',
+        port: 3035
+      }
+    })
+  })
+
+  test('with a non-standard env', () => {
+    process.env.NODE_ENV = 'staging'
+    process.env.RAILS_ENV = 'staging'
+    const { environment } = require('../index')
+    expect(environment.toWebpackConfig()).toMatchObject({
+      devtool: 'nosources-source-map',
+      stats: 'normal'
+    })
+  })
+})

package/config.js

@@ -3,41 +3,25 @@ const { safeLoad } = require('js-yaml')
 const { readFileSync } = require('fs')
 const deepMerge = require('./utils/deep_merge')
 const { isArray } = require('./utils/helpers')
+const env = require('./env')
 
-const defaultFilePath = require.resolve('../lib/install/config/webpacker.yml')
-const filePath = resolve('config', 'webpacker.yml')
+const defaultConfigPath = require.resolve('../lib/install/config/webpacker.yml')
+const configPath = resolve('config', 'webpacker.yml')
 
-const environment = process.env.NODE_ENV || 'development'
-const defaultConfig = safeLoad(readFileSync(defaultFilePath), 'utf8')[environment]
-const appConfig = safeLoad(readFileSync(filePath), 'utf8')[environment]
+const getConfig = () => {
+  const defaults = safeLoad(readFileSync(defaultConfigPath), 'utf8')[env]
+  const app = safeLoad(readFileSync(configPath), 'utf8')[env]
 
-if (isArray(appConfig.extensions) && appConfig.extensions.length) {
-  delete defaultConfig.extensions
-} else {
-  /* eslint no-console: 0 */
-  console.warn('No extensions specified in webpacker.yml, using default extensions\n')
-}
-
-const config = deepMerge(defaultConfig, appConfig)
+  if (isArray(app.extensions) && app.extensions.length) {
+    delete defaults.extensions
+  }
 
-const isBoolean = str => /^true/.test(str) || /^false/.test(str)
+  const config = deepMerge(defaults, app)
 
-const fetch = key =>
-  (isBoolean(process.env[key]) ? JSON.parse(process.env[key]) : process.env[key])
+  config.outputPath = resolve('public', config.public_output_path)
+  config.publicPath = `/${config.public_output_path}/`.replace(/([^:]\/)\/+/g, '$1')
 
-const devServer = (key) => {
-  const envValue = fetch(`WEBPACKER_DEV_SERVER_${key.toUpperCase().replace(/_/g, '')}`)
-  if (typeof envValue === 'undefined' || envValue === null) return config.dev_server[key]
-  return envValue
+  return config
 }
 
-if (config.dev_server) {
-  Object.keys(config.dev_server).forEach((key) => {
-    config.dev_server[key] = devServer(key)
-  })
-}
-
-config.outputPath = resolve('public', config.public_output_path)
-config.publicPath = `/${config.public_output_path}/`.replace(/([^:]\/)\/+/g, '$1')
-
-module.exports = config
+module.exports = getConfig()

package/config_types/__tests__/config_list.js

@@ -8,11 +8,6 @@ test('new', () => {
   expect(list).toBeInstanceOf(Array)
 })
 
-test('set', () => {
-  const list = new ConfigList()
-  expect(list.set('key', 'value')).toEqual([{ key: 'key', value: 'value' }])
-})
-
 test('get', () => {
   const list = new ConfigList()
   list.append('key', 'value')

package/config_types/config_list.js

@@ -10,15 +10,6 @@ class ConfigList extends Array {
     return this[index].value
   }
 
-  /**
-    * @deprecated after the 3.0.2 release and will be removed in the next major release
-  */
-  set(key, value) {
-    /* eslint no-console: 0 */
-    console.warn('set is deprecated! Use append instead')
-    return this.append(key, value)
-  }
-
   append(key, value) {
     return this.add({ key, value })
   }

package/dev_server.js

@@ -0,0 +1,23 @@
+const { isBoolean, isEmpty } = require('./utils/helpers')
+const config = require('./config')
+
+const fetch = (key) => {
+  const value = process.env[key]
+  return isBoolean(value) ? JSON.parse(value) : value
+}
+
+const devServer = () => {
+  const devServerConfig = config.dev_server
+
+  if (devServerConfig) {
+    Object.keys(devServerConfig).forEach((key) => {
+      const envValue = fetch(`WEBPACKER_DEV_SERVER_${key.toUpperCase().replace(/_/g, '')}`)
+      if (isEmpty(envValue)) return devServerConfig[key]
+      devServerConfig[key] = envValue
+    })
+  }
+
+  return devServerConfig || {}
+}
+
+module.exports = devServer()

package/env.js

@@ -0,0 +1,23 @@
+const { resolve } = require('path')
+const { safeLoad } = require('js-yaml')
+const { readFileSync } = require('fs')
+
+const configPath = resolve('config', 'webpacker.yml')
+const DEFAULT_ENV = 'production'
+
+const env = () => {
+  const nodeEnv = process.env.NODE_ENV
+  const railsEnv = process.env.RAILS_ENV
+  const config = safeLoad(readFileSync(configPath), 'utf8')
+  const availableEnvironments = Object.keys(config).join('|')
+  const regex = new RegExp(availableEnvironments, 'g')
+
+  if (nodeEnv && nodeEnv.match(regex)) return nodeEnv
+  if (railsEnv && railsEnv.match(regex)) return railsEnv
+
+  /* eslint no-console: 0 */
+  console.warn(`NODE_ENV=${nodeEnv} and RAILS_ENV=${railsEnv} environment is not defined in config/webpacker.yml, falling back to ${DEFAULT_ENV}`)
+  return DEFAULT_ENV
+}
+
+module.exports = env()

package/__tests__/environment.js → package/environments/__tests__/base.js

@@ -3,14 +3,14 @@
 // environment.js expects to find config/webpacker.yml and resolved modules from
 // the root of a Rails project
 
-const chdirApp = () => process.chdir('test/test_app')
-const chdirCwd = () => process.chdir(process.cwd())
-chdirApp()
+const { chdirTestApp, chdirCwd } = require('../../utils/helpers')
+
+chdirTestApp()
 
 const { resolve } = require('path')
-const rules = require('../rules')
-const { ConfigList } = require('../config_types')
-const Environment = require('../environment')
+const rules = require('../../rules')
+const { ConfigList } = require('../../config_types')
+const Environment = require('../base')
 
 describe('Environment', () => {
   afterAll(chdirCwd)

package/environment.js → package/environments/base.js

@@ -12,9 +12,9 @@ const ExtractTextPlugin = require('extract-text-webpack-plugin')
 const ManifestPlugin = require('webpack-manifest-plugin')
 const CaseSensitivePathsPlugin = require('case-sensitive-paths-webpack-plugin')
 
-const { ConfigList, ConfigObject } = require('./config_types')
-const rules = require('./rules')
-const config = require('./config')
+const { ConfigList, ConfigObject } = require('../config_types')
+const rules = require('../rules')
+const config = require('../config')
 
 const getLoaderList = () => {
   const result = new ConfigList()
@@ -85,7 +85,7 @@ const getBaseConfig = () =>
     }
   })
 
-module.exports = class Environment {
+module.exports = class Base {
   constructor() {
     this.loaders = getLoaderList()
     this.plugins = getPluginList()

package/environments/development.js

@@ -1,8 +1,9 @@
 const webpack = require('webpack')
-const Environment = require('../environment')
-const { dev_server: devServer, outputPath: contentBase, publicPath } = require('../config')
+const Base = require('./base')
+const devServer = require('../dev_server')
+const { outputPath: contentBase, publicPath } = require('../config')
 
-module.exports = class extends Environment {
+module.exports = class extends Base {
   constructor() {
     super()