Skip to content

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

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

Merged
merged 6 commits into from
Feb 2, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
1 change: 1 addition & 0 deletions lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ module.exports = function loader (css, map) {
return null
})
}).catch((err) => {
if (err.file) { this.addDependency(err.file) }
Copy link
Member

Choose a reason for hiding this comment

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

if (err.file) this.addDependency(file)

return ...

return err.name === 'CssSyntaxError' ? cb(new SyntaxError(err)) : cb(err)
})
}
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,12 @@
"jest": "^21.0.0",
"jsdoc-to-markdown": "^3.0.0",
"memory-fs": "^0.4.0",
"postcss-import": "^11.0.0",
"postcss-js": "^1.0.0",
"standard": "^10.0.0",
"standard-version": "^4.0.0",
"sugarss": "^1.0.0",
"util.promisify": "^1.0.0",
"webpack": "^3.0.0"
},
"scripts": {
Expand Down
6 changes: 6 additions & 0 deletions test/__snapshots__/loader.test.js.snap
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Loader Default 1`] = `"module.exports = \\"a { color: black }\\\\n\\""`;

exports[`Loader Watching Deps After An Error Default 1`] = `"module.exports = \\"a { color: black }\\\\n\\""`;

exports[`Loader Watching Deps After An Error Default 2`] = `"throw new Error(\\"Module build failed: Syntax Error \\\\n\\\\n(1:5) Unknown word\\\\n\\\\n\\\\u001b[31m\\\\u001b[1m>\\\\u001b[22m\\\\u001b[39m\\\\u001b[90m 1 | \\\\u001b[39ma \\\\u001b[33m{\\\\u001b[39m color black \\\\u001b[33m}\\\\u001b[39m\\\\n \\\\u001b[90m | \\\\u001b[39m \\\\u001b[31m\\\\u001b[1m^\\\\u001b[22m\\\\u001b[39m\\\\n \\\\u001b[90m 2 | \\\\u001b[39m\\\\n\\");"`;

exports[`Loader Watching Deps After An Error Default 3`] = `"module.exports = \\"a { color: black }\\\\n\\""`;
3 changes: 3 additions & 0 deletions test/fixtures/css-watching/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import style from './style.css'

export default style
1 change: 1 addition & 0 deletions test/fixtures/css-watching/noSyntaxError.css
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
a { color: black }
1 change: 1 addition & 0 deletions test/fixtures/css-watching/style.css
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
@import "./styleDep";
1 change: 1 addition & 0 deletions test/fixtures/css-watching/syntaxError.css
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
a { color black }
20 changes: 15 additions & 5 deletions test/helpers/compiler.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,21 @@ module.exports = function compiler (fixture, config, options) {

if (!options.emit) compiler.outputFileSystem = new MemoryFS()

return new Promise((resolve, reject) => {
return compiler.run((err, stats) => {
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

options.handler(err, stats, (s) => {
c.close(resolve)
})
})
})
} else {
return new Promise((resolve, reject) => {
return compiler.run((err, stats) => {
if (err) reject(err)

resolve(stats)
resolve(stats)
})
})
})
}
}
31 changes: 31 additions & 0 deletions test/helpers/fileChange.js
Original file line number Diff line number Diff line change
@@ -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 { readFile, writeFile, unlink } = require('fs')
const promisify = require('util.promisify')

const rf = promisify(readFile)
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 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


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

}

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

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

return wf(fileName, contents)
}

module.exports.copyCssFile = function copyCssFile (src, dest) {
Copy link
Member

Choose a reason for hiding this comment

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

copyCssFile => copy

return readCssFile(src)
.then(contents => writeCssFile(dest, contents))
}

module.exports.deleteCssFile = function deleteCssFile (name) {
Copy link
Member

Choose a reason for hiding this comment

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

deleteCssFile => delete

Copy link
Contributor Author

Choose a reason for hiding this comment

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

delete is a reserved keyword, want me to call the functions deleteFile, copyFile, etc?

Copy link
Member

Choose a reason for hiding this comment

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

Ooops 😛 yeah right... *File is 👍

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

return rm(fileName)
}
59 changes: 59 additions & 0 deletions test/loader.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

const webpack = require('./helpers/compiler')
const { loader } = require('./helpers/compilation')
const { copyCssFile, deleteCssFile } = require('./helpers/fileChange');
Copy link
Member

Choose a reason for hiding this comment

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

const { copy, delete } = require('./helpers/fs')


describe('Loader', () => {
test('Default', () => {
Expand All @@ -20,4 +21,62 @@ describe('Loader', () => {
expect(src).toMatchSnapshot()
})
})

describe('Watching Deps After An Error', () => {
Copy link
Member

Choose a reason for hiding this comment

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

describe('Watching', () => {
   describe('Dependencies', () => {
     test('Error', () => {
        // ...
     })
   })
})

const files = {
syntaxError: "css-watching/syntaxError.css",
noSyntaxError: "css-watching/noSyntaxError.css",
changingFile: "css-watching/styleDep.css"
}

beforeAll(() => copyCssFile(files.noSyntaxError, files.changingFile))

afterAll(() => deleteCssFile(files.changingFile))

test('Default', () => {
const config = {
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.

}
}
}

const testSteps = [
Copy link
Member

Choose a reason for hiding this comment

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

testSteps => steps

(stats) => {
const { err, src } = loader(stats)
Copy link
Member

Choose a reason for hiding this comment

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

\n

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

return copyCssFile(files.syntaxError, files.changingFile)
},
(stats) => {
const { err, src } = loader(stats)
expect(src).toMatchSnapshot()
expect(err.length).toEqual(1)
return copyCssFile(files.noSyntaxError, files.changingFile)
},
(stats, close) => {
const { err, src } = loader(stats)
expect(src).toMatchSnapshot()
expect(src).toEqual("module.exports = \"a { color: black }\\n\"")
expect(err.length).toEqual(0)
return close()
}
];

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 }
   }
}

handler: (err, stats, close) => {
testSteps[currentStep](stats, close)
currentStep++
}
}

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)

})
})

})