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

fix(index): continue watching after dependency {Error} #332

Merged
merged 6 commits into from
Feb 2, 2018

Conversation

zenbrent
Copy link
Contributor

@zenbrent zenbrent commented Feb 1, 2018

ℹ️ Describe the big picture of your changes here to communicate to the maintainers
why we should accept this pull request. If it fixes a bug or resolves a feature
request, be sure to link to that issue.

When you are watching a file that has dependancies (in my case, I was using the postcss-import module), and you save a file with a syntax error, the file watcher stops working.
This PR makes webpack continue watching in that case.

Type


ℹ️ What types of changes does your code introduce?

Put an x in the boxes that apply

  • Fix
  • Perf
  • Docs
  • Test
  • Chore
  • Feature
  • Refactor

SemVer


ℹ️ What changes to the current semver range does your PR introduce?

Put an x in the boxes that apply

  • Bug (:label: Patch)
  • Feature (:label: Minor)
  • Breaking Change (:label: Major)

Issues


ℹ️ What issue (if any) are closed by your PR?

Replace #1 with the error number that applies

Couldn't find an issue for this.

Checklist


ℹ️ You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. This is a reminder of what we are going to look for before merging your code.

Put an x in the boxes that apply.

  • Lint and unit tests pass with my changes
  • I have added tests that prove my fix is effective/works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes are merged and published in downstream modules

@coveralls
Copy link

coveralls commented Feb 1, 2018

Coverage Status

Coverage increased (+0.9%) to 88.235% when pulling 6a6968d on zenbrent:master into 08c063a on postcss:master.

@michael-ciniawsky michael-ciniawsky changed the title Continue watching after an error in a dependency fix(index): continue watching after dependency {Error} Feb 1, 2018
Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

👍 After Code Style fixes, I will land this

@@ -0,0 +1,31 @@
const path = require('path')
Copy link
Member

Choose a reason for hiding this comment

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

test/helpers/fileChange => test/helpers/fs

const wf = promisify(writeFile)
const rm = promisify(unlink)

function readCssFile (name) {
Copy link
Member

Choose a reason for hiding this comment

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

readCssFile => read

const rm = promisify(unlink)

function readCssFile (name) {
const fileName = path.join(__dirname, '../fixtures', name)
Copy link
Member

Choose a reason for hiding this comment

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

fileName => file

const fileName = path.join(__dirname, '../fixtures', name)

return rf(fileName)
.then(c => c.toString())
Copy link
Member

Choose a reason for hiding this comment

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

c => content || data

.then(c => c.toString())
}

function writeCssFile (name, contents) {
Copy link
Member

Choose a reason for hiding this comment

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

writeCssFile => write

(stats) => {
const { err, src } = loader(stats)
expect(src).toMatchSnapshot()
expect(err.length).toEqual(0)
Copy link
Member

Choose a reason for hiding this comment

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

\n

Copy link
Member

Choose a reason for hiding this comment

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

For each element in steps please

var currentStep = 0

const options = {
watching: true,
Copy link
Member

Choose a reason for hiding this comment

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

Either

const options = {
  watch (err, stats, close) {
     // ...handling
  }
}

or

const options = {
   watch: {
      handler (err, stats, close) { // ...handling }
   }
}

}
}

return webpack('css-watching/index.js', config, options)
Copy link
Member

Choose a reason for hiding this comment

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

css-watching => watch/watching (Fixtures)

if (err) reject(err)
if (options.watching) {
return new Promise((resolve, reject) => {
const c = compiler.watch({}, (err, stats) => {
Copy link
Member

Choose a reason for hiding this comment

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

c => watcher

loader: {
options: {
plugins: [require("postcss-import")],
watching: true
Copy link
Member

Choose a reason for hiding this comment

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

This should be config.watch(ing) instead of config.loader.watch(ing) ?

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 catch, it isn't needed at all since I'm calling .watch() on the compiler.

@zenbrent
Copy link
Contributor Author

zenbrent commented Feb 1, 2018

That should cover everything.

Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

@zenbrent Thx. If you like PR the watch logic of yours to webpack-contrib/test-utils, otherwise I will 'steal this feature' 😛

@michael-ciniawsky michael-ciniawsky merged commit a8921cc into webpack-contrib:master Feb 2, 2018
@michael-ciniawsky
Copy link
Member

Released in v2.1.0 🎉

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

Successfully merging this pull request may close these issues.

3 participants