Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from
+1 −1
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const environment = process.env.NODE_ENV || 'development'
const defaultConfig = safeLoad(readFileSync(defaultFilePath), 'utf8')[environment]
const appConfig = safeLoad(readFileSync(filePath), 'utf8')[environment]

if (isArray(appConfig.extensions) && appConfig.extensions.length) {
if (isArray(appConfig.extensions) && appConfig.extensions.length && defaultConfig) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like the best solution to me

+1 👏

delete defaultConfig.extensions
} else {
/* eslint no-console: 0 */
Expand Down