refactor: apply webpack defaults #51
base: master
Are you sure you want to change the base?
Conversation
src/index.js
Outdated
} | ||
|
||
let result; | ||
if (query.lazy) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe rename lazy
=> async
, bc of a proposed sync
option (#50) and therefore for consistency? And it is semver major anyways :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Lazy references a common naming convention for
loading
of something. - Async is about order of execution or lack there of to be more specific.
Within this particular context i think lazy
is the correct name for the query option. Given a bundle is either loader lazy
or not
I don't see the need to define a sync
option given it's the only other practical alternative to lazy
and the default if you don't explicitly lazy
load the bundle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there will never be a sync option, because it doesn't make sense then leaving lazy
is better yep :).
src/index.js
Outdated
import loaderUtils from 'loader-utils'; | ||
|
||
export function pitch(remainingRequest) { // eslint-disable-line no-unused-vars | ||
const query = loaderUtils.getOptions(this) || {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick 😛 query
=> options
Codecov Report
@@ Coverage Diff @@
## master #51 +/- ##
======================================
Coverage ? 0%
======================================
Files ? 2
Lines ? 14
Branches ? 4
======================================
Hits ? 0
Misses ? 11
Partials ? 3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just 2 nits
{ | ||
"useBuiltIns": true, | ||
"targets": { | ||
"node": "4.3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4.8
' }\n', | ||
`}${chunkNameParam});`]; | ||
} | ||
return result.join(''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
join('\n')
and remove the \n
foreach item? Can be follow up style PR though
We need to be careful with what is exported here aswell as the main part is the named export for the pitching loader |
I was actually just looking at that. iirc etwp is the same way? no? |
Any way it goes, i'm not merging it. I'll push a beta directly from this branch. I'm not big on majors without a supporting test suite |
src/cjs.js const loader = require('./index');
module.exports = loader.default;
module.exports.pitch = loader.pitch; ? Not 💯 :) |
I'm 90% sure you are right about that |
Codecov Report
@@ Coverage Diff @@
## master #51 +/- ##
========================================
Coverage ? 0.00%
========================================
Files ? 2
Lines ? 14
Branches ? 4
========================================
Hits ? 0
Misses ? 11
Partials ? 3 Continue to review full report at Codecov.
|
Intended to be merged & released as a part of
1.0.0
on a betadist-tag
once this has been finished and properly tested.BREAKING CHANGE:
Enforces NodeJS > 4.3 via engines
Closes #43