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.

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

Declare processConfigsErr before use #2858

Merged
merged 6 commits into from May 17, 2019
Merged

Conversation

humphd
Copy link
Contributor

@humphd humphd commented May 15, 2019

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

I was following a suggestion from sveltejs/template#16 (comment) to add clearScreen: false to my rollup config.

Description

This fix deals with the case that a user specifies watch: { clearScreen: false } in their config, which currently causes the following error to be thrown:

ReferenceError: processConfigsErr is not defined
    at configs.map.options (/Users/humphd/Desktop/svelte-app/node_modules/rollup/b
in/rollup:1460:35)
    at Array.map (<anonymous>)
    at processConfigs (/Users/humphd/Desktop/svelte-app/node_modules/rollup/bin/ro
llup:1444:24)
    at watch (/Users/humphd/Desktop/svelte-app/node_modules/rollup/bin/rollup:1435
:28)
    at execute (/Users/humphd/Desktop/svelte-app/node_modules/rollup/bin/rollup:16
65:9)
    at loadConfigFile.then.configs (/Users/humphd/Desktop/svelte-app/node_modules/
rollup/bin/rollup:1656:30)
    at process._tickCallback (internal/process/next_tick.js:43:7)
    at Function.Module.runMain (internal/modules/cjs/loader.js:763:11)
    at startup (internal/bootstrap/node.js:303:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:872:3)

The problem here is that processConfigsErr is being set via processConfigs() before it is declared. Moving this declaration up before the call to processConfigs() fixes it.

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Thanks a lot for spotting this! Just for sport, I will see if there is an easy way to add a test before merging this as this is somewhat embarrassing ...

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

I managed to add a test that at least fails with the previous version and which might serve as a template for more tests of the watch CLI in the future. From my side, this is good to go once CI is green.

@lukastaegert lukastaegert merged commit 7aaec61 into rollup:master May 17, 2019
@humphd
Copy link
Contributor Author

humphd commented May 17, 2019

@lukastaegert thanks writing a test for this. I wasn't sure how to add one for this, and felt badly not including one, but wanted to get the fix in regardless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants