Fix compilation for environments that are not test, development or production

[rmehner]

The changes introduced with #1253 (and therefore v3.2.2) will fail the compilation with:

TypeError: Cannot convert undefined or null to object

if the environment is something that is not in the default configs, for
example "staging". With this change, we don't try to delete from the
defaultConfig if there is no default config.

Test suite didn't work on my machine at all and I'm a bit short on time right now, but given that this fix is relatively straight forward, I thought it might be not too bad :)

[hiromi2424]

else block won't be executed when appConfig has no extensions and no defaultConfig found.
Such will avoid warnings for appConfig.extensions.
How about moving defaultConfig check inside into delete defaultConfig.extensions and showing warning if default config is empty?

if (isArray(appConfig.extensions) && appConfig.extensions.length) {
  if (defaultConfig) {
    delete defaultConfig.extensions
  } else {
    console.warn('No default config was found\n')
  }
} else {
  /* eslint no-console: 0 */
  console.warn('No extensions specified in webpacker.yml, using default extensions\n')
}

Also warning of empty defaultConfig might be better to place on another place though

[rmehner]

Good point, but is it really a problem that the else block is not run, given that it has no effects apart from the console warning anyway (and since there is no default config, the message is not entirely accurate then)?

Happy to adapt, just trying to understand and figuring out the most user friendly way :)

[hiromi2424]

It seems @IgorDmitriev 's solitions is better than this way 😄

[doits]

I'd only skip the delete part and output no warning. I mean what the code does is to delete the default extensions if custom ones are specified. If there is no default config, there are no default extensions, so everything is fine and there is no need to delete anything.

if (isArray(appConfig.extensions) && appConfig.extensions.length) {
  if (defaultConfig) {
    delete defaultConfig.extensions
  }
} else {
  /* eslint no-console: 0 */
  console.warn('No extensions specified in webpacker.yml, using default extensions\n')
}

To be more precise, currently one could add a check whether there are default extensions and log an error if there aren't:

if (isArray(appConfig.extensions) && appConfig.extensions.length) {
  if (defaultConfig) {
    delete defaultConfig.extensions
  }
} else if(defaultConfig && isArray(defaultConfig.extensions)) {
  /* eslint no-console: 0 */
  console.warn('No extensions specified in webpacker.yml, using default extensions\n')
} else {
  /* eslint no-console: 0 */
  console.error('No extensions specified in webpacker.yml nor default config, compiling will probably fail\n')
}

If there should be a fallback default config etc. is another topic, but if it takes longer to settle on this question I'd suggest releasing the fix in a minor update asap into the wild so we can use the latest webpacker version and adding a fallback in version 3.3 (because it changes behavior).

[rmehner]

@doits the code you have in your example is exactly what my code does right now, sans the warning, so I guess we have come to some sort of conclusion :)

I'd suggest releasing the fix in a minor update asap into the wild so we can use the latest webpacker version and adding a fallback in version 3.3 (because it changes behavior).

This sounds like the best solution to me, because right now I have to pin webpacker to an old version to not run into this issue.

[hiromi2424]

This sounds like the best solution to me

+1 👏

[IgorDmitriev]

As an alternative solution, what if defaultConfig would fall back to production default config if the current environment is not found? Since I would expect my staging/qa environments to be configured production-like and inherit all default options before merging appConfig.

const loadDefaultConfig = environment => {
  const config = safeLoad(readFileSync(defaultFilePath), 'utf8');
  
  if (config[environment] !== undefined) return config[environment];
 
  console.warn(`No default config was found for ${environment}. Using production default config.`);
  return config['production'];
};

const defaultConfig = loadDefaultConfig(environment);

[rmehner]

@IgorDmitriev Yes, I actually think that's the better way, however it's also a change in behaviour and is making an assumption that we always wanna derive from production-like environments.

The current behaviour is (without the bug that #1253 introduced), to take all of the app config and assume default config is empty, therefore not providing any defaults at all.

With the changes you mentioned, we'll actually have a default config that is based on the production settings and if we don't have that overwritten by the app config, they will be in effect.

E.g. in production we'd have cache_manifest set to false and if we don't explicitly overwrite this in our appConfig, it will stay false. However the current behaviour is that cache_manifest is undefined and therefore some default higher up in the chain will be in effect.

Since we're consoling out a warning, I guess this is fine, but just wanted to note this subtle change in behaviour before we go anywhere :)

Also, there's a default environment, wonder if we should take that instead.

Thank you @IgorDmitriev and @hiromi2424 for your excellent suggestions & comments :)

[gauravtiwari]

Thanks @rmehner for the PR and everyone for suggestions. Apologies for delay. The gem already has same logic for getting environment but not for npm package. I am working on a PR to get this issue resolved soon. Thanks for your patience.