Include "test" in environment whitelist

[schpet]

without this i don't think package/environments/test.js is ever loaded.

https://github.com/schpet/webpacker/blob/include-test-environment-in-whitelist/package/index.js#L13

#1502 possibly related?

[guilleiguaran]

@schpet I'm going to test this in my app and report here if this fixes it 😊

[edslocomb]

Ohhhh, so that's why things aren't working...

Maybe instead of hard-coding the environment list in 'NODE_ENVIRONMENTS' (which I suspect is/was meant to correspond to the webpack --mode setting, which has only those two legal values) this should read config/webpack/*.js and allow any of those except environment.js?

[edslocomb]

EDIT: The fix below is available in a fork-branch, if that's of any interest.

Here's what I've got for package/env.js after refactoring to read config/webpack/** instead of relying on hard-coding:

const { resolve } = require('path')                                                                         
const { safeLoad } = require('js-yaml')                                                                     
const { readdirSync, readFileSync } = require('fs')                                                         
                                                                                                            
const DEFAULT = 'production'                                                                                
const ymlConfigPath = resolve('config', 'webpacker.yml')                                                    
                                                                                                            
const railsEnv = process.env.RAILS_ENV                                                                      
const nodeEnv = process.env.NODE_ENV                                                                        
                                                                                                            
const ymlConfig = safeLoad(readFileSync(ymlConfigPath), 'utf8')                                             
const availableYmlEnvironments = Object.keys(ymlConfig)                                                     
                                       .filter((key) => key !== 'default')                                  
                                                                                                            
const jsConfigDir = resolve('config', 'webpack')                                                            
const jsConfigs = readdirSync(jsConfigDir)                                                                  
  .map((f) => f.replace(/\.[^\.]+$/, ''))                                                                   
  .filter((f) => f !== 'environment')                                                                       
                                                                                                            
module.exports = {                                                                                          
  railsEnv: railsEnv && availableYmlEnvironments.includes(railsEnv) ? railsEnv : DEFAULT,                   
  nodeEnv: nodeEnv && jsConfigs.includes(nodeEnv) ? nodeEnv : DEFAULT                                       
}                                                                                                           

[gauravtiwari]

Thanks, @schpet.

I forgot to remove the test.js file earlier but anyway, the idea behind this was since webpack and most of the JS libraries only understands two environments, we don't need a test environment. Ideally, we want to be testing or running CI against development or production bundle, i.e. the bundle produced uses the same webpack config as production bundle. Having totally different configs could potentially break the build on production although it might be passing on CI.

What do you think?

[schpet]

hey folks, thanks for checking this out!

@gauravtiwari that makes sense, appreciate the context.

the problem i was facing is that react throws a warning because my NODE_ENV !== 'production' but the JS was minified:

Warning: It looks like you're using a minified copy of the development build of React. When deploying React apps to production, make sure to use the production build which skips development warnings and is faster. See https://fb.me/react-minification for more details.

i'm worried that webpacker's production defaults will result in other people seeing this console error. what do you think of setting mode: "production" only in production.js? and keeping test.js so that by default tests will not be minified. folks who want to test production JS can precompile assets or set NODE_ENV=production. @edslocomb's PR #1514 supports the idea that the prod setup isn't ideal for tests.

in my app, i've added the following workaround to config/webpack/test.js to make the react error go away:

const environment = require("./environment")

environment.config.merge({
  mode: "development"
})

module.exports = environment.toWebpackConfig()

interested in your thoughts on this 😄

[edslocomb]

@gauravtiwari I see where you're coming from, but in Rails there are already a lot of things done differently in the test environment than in production, and Rails users are accustomed to having the production and test environments neatly divided up into separate files.

Of course you don't need to that to achieve the differences you need for your project -- you can just use conditionals directly in your environment.js file to test process.env.RAILS_ENV and/or process.env.NODE_ENV -- but that's a highly un-Rails-like way to handle configuration.

I suppose you do have to choose between Rails customs and Webpack customs at some point, but I'd lean toward Rails in Webpacker-- it is part of the Rails project, after all :)

EDIT:

I'd also note that it's fairly common for large-ish Rails projects to add a new environment at some point for some special purpose or other-- in my travels I've seen a 'staging' environment that was almost but not quite like production, and a 'profile' environment for load testing.

People who have seen and done this sort of thing before are going to expect to be able to pick a name for the new environment, drop a config file with that name into the appropriate folder, and have everything Just Work.

[schpet]

closing this in favor of #1524 based on @gauravtiwari's feedback.

i finally spun up a fresh app and realized that by default, the development environment is used, so I think these defaults work well 👍🏻

[schpet]

@edslocomb you wrote:

People who have seen and done this sort of thing before are going to expect to be able to pick a name for the new environment, drop a config file with that name into the appropriate folder, and have everything Just Work.

AFAIK this still works, e.g.

$ cat config/webpack/foobar.js
throw new Error('foobar loaded')

$ NODE_ENV=foobar bin/webpack
/private/tmp/webpacker-may-25/config/webpack/foobar.js:1
(function (exports, require, module, __filename, __dirname) { throw new Error('foobar loaded')
                                                              ^

Error: foobar loaded
    at Object.<anonymous> (/private/tmp/webpacker-may-25/config/webpack/foobar.js:1:69)
    at Module._compile (internal/modules/cjs/loader.js:678:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:689:10)
    at Module.load (internal/modules/cjs/loader.js:589:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:528:12)
    at Function.Module._load (internal/modules/cjs/loader.js:520:3)
    at Module.require (internal/modules/cjs/loader.js:626:17)
    at require (internal/modules/cjs/helpers.js:20:18)
    at requireConfig (/private/tmp/webpacker-may-25/node_modules/webpack/bin/convert-argv.js:97:18)
    at /private/tmp/webpacker-may-25/node_modules/webpack/bin/convert-argv.js:104:17

am I missing something?

[gauravtiwari]

That's correct @schpet

Please see #1359 for more context in regards to using custom environments in Webpacker.

[guilleiguaran]

@gauravtiwari @schpet can you help me to figure out what we should do to get #1502 working?

Does #1524 fix it? Should I move my configs from test.js to development.js?

[gauravtiwari]

@guilleiguaran What do you have in your test.js file? Just curious to see your setup

Should I move my configs from test.js to development.js?

yes, please

[guilleiguaran]

@guilleiguaran What do you have in your test.js file? Just curious to see your setup

const environment = require('./environment')
const webpack = require('webpack')

// We must expose jQuery to the global object to be able to use `JQuery.active`
// in `wait_for_ajax` (spec/support/wait_for_ajax.rb)

environment.loaders.append('expose', {
  test: require.resolve('jquery'),
  use: {
    loader: 'expose-loader',
    options: 'jQuery'
  }
})

module.exports = environment.toWebpackConfig()

That is required for the Capybara described here: https://robots.thoughtbot.com/automatically-wait-for-ajax-with-capybara

[gauravtiwari]

Thanks, @guilleiguaran If you move this code to development.js, is everything good on CI and locally?

I am also thinking if there is a good case to bring back test.js, although I am a bit worried about unreliable tests if different configs are used - test vs production bundle.