refactor: apply webpack-defaults#25
Conversation
michael-ciniawsky
changed the title
webpack-defaults
26270e2 to
793183a
Compare
951390f to
0a77ec0
Compare
Codecov Report
@@ Coverage Diff @@
## master #25 +/- ##
=========================================
Coverage ? 66.66%
=========================================
Files ? 2
Lines ? 3
Branches ? 0
=========================================
Hits ? 2
Misses ? 1
Partials ? 0
Continue to review full report at Codecov.
|
michael-ciniawsky
left a comment
michael-ciniawsky
left a comment
There was a problem hiding this comment.
This should be good to go, the comments could be adressed or ignored and the follow up PR with standardize test setup just refactors/cleans them up. But #36 should land as soon as possible
| @@ -0,0 +1,81 @@ | |||
| import path from 'path'; | |||
There was a problem hiding this comment.
Maybe we should leave this out for now until there is consensus about the initial test setup
| @@ -0,0 +1,81 @@ | |||
| import path from 'path'; | |||
| import webpack from 'webpack'; | |||
| import Promise from 'bluebird'; | |||
There was a problem hiding this comment.
Please no extra lib for {Promise} as every supported target has native {Promise} support
promisify() can easily be polyfilled by a custom helper or a lightweight lib || utils.promisify()
There was a problem hiding this comment.
See comment below & I agree with you entirely. In fact I asked Sean about that shortly after looking at the example.
utils.promisify() iirc isn't available in all our supported Node versions so if we were going to use this setup ( and i'm not ) i was thinking about something like pify.
| const stats = await run(); | ||
|
|
||
| const { compilation } = stats; | ||
| const { errors, warnings, assets, entrypoints, chunks, modules } = compilation; |
There was a problem hiding this comment.
Destructuring here and returning an {Object} on L69 doesn't really make sense, its better to return the whole compilation and pass it to helpers in the particular test
example.test.js
import webpack from 'helpers/compiler'
import { loader, errors, warnings, assets } from 'helpers/utils' // maybe test-utils/stats-utils
test('Loader', () => {
const config = { loader: {...options} }
webpack('path/to/fixture.ext', config)
.then((compilation) => loader(compilation))
.then({ result, map, meta } => {
expect(result).toMatchSnapshot()
if (map) expect(map).toMatchSnapshot()
...
})
.catch((err) => err)
})There was a problem hiding this comment.
But thats discussion for webpack-defaults #68 to not derail the PR here further :)
| }); | ||
|
|
||
| test('should load scss file', async () => { | ||
| const { result } = await runLoadersPromise({ resource: getFixtureResource(fixtureName), loaders }); |
There was a problem hiding this comment.
Maybe just two simple tests (default/escaping(#36)) with loader.call(ctx, content) for now as the raw-loader doesn't demand any requirements and proper testing in a follow up
There was a problem hiding this comment.
@michael-ciniawsky - Yeah, I was just tinkering with Seans test setup which is complete overkill here but how cumbersome it was in a small repo was what I was curious about.
I'm going to lean this out a bunch more inline with your initial pass
michael-ciniawsky
added
pr_action: Merge ready
status: Approved
and removed
flag: Needs Tests
pr_action: Review
labels
2 participants
Intended to be merged & released as a part of
1.0.0on a betadist-tagonce this has been finished and properly tested.BREAKING CHANGE:
Enforces NodeJS > 4.8 via engines
Closes #25